-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[v3] Complexity cost bug fixes #4843
base: master
Are you sure you want to change the base?
Conversation
|
||
# Compute maximum possible cost of child selections; | ||
# composites merge their maximums, while leaf scopes are always zero. | ||
# FieldsWillMerge validation assures all scopes are uniformly one or the other. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With FieldsWillMerge
getting fixed, this should now be consistent about always having scopes of the same kind. Either way, it's written to be fault-tolerant in the event that it's run without first validating.
|
||
# Identify the maximum cost and scope among possibilities | ||
maximum_cost = 0 | ||
maximum_scope = child_scopes.reduce(child_scopes.last) do |max_scope, possible_scope| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This switches to always hill-climb for the maximum possibility among scopes, regardless if they are leaf or composite scopes.
field_cost = composite_scopes.last.own_complexity(child_complexity) | ||
field_complexity(composite_scopes.last, max_complexity: field_cost, child_complexity: child_complexity) | ||
end | ||
field_complexity( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lastly, always trigger exactly one hook per field with the resolved maximums among scopes, costs, and children costs. Before leaf scopes would receive nil
as their child_complexity
– now they receive 0
. This feels more inline with the own_complexity
call that has always sent zero child cost to leaf scopes. If the hook cares to differentiate, it can always inspect the scope to determine if there are child selections.
bd94574
to
d325c38
Compare
Hey, thanks so much for this detailed report and fix. As you can see, it's been a while since I dug into the complexity implementation! I'll review these closely soon. |
I noticed a
v3
milestone, so thought this would be a good time to submit a fix for several bugs in the complexity algorithm that have the potential to increase costs (slightly and rarely) and thus would technically be breaking.Here's what's been weird...
The same issue as above also happened with merged leaf fields, but for a different reason. Leaf possibilities always directly assigned their field cost rather than hill-climbing for a maximum. Once again, last selection wins.
Leaf fields would trigger hooks N-times for each leaf possibility, when really we should expect only one hook per field.
Compounding upon item 3, there was also a crazy side effect here from Type comparison missing from
FieldsWillMerge
validation #4403. BecauseFieldsWillMerge
was faulty about type consistency (fix slated for3.0
), it meant that the fields being merged could intermix leaf and composite scopes – which each had their own workflow for triggering hooks. So, in addition to leaf field hooks triggering multiple times, an additional hook could trigger once for composite possibilities.