Turn a negated difference round wherever it sits (#882) - #905
Merged
Conversation
-(a - b) became b - a for a whole expression and not for the same expression inside another node, so what a caller got depended on where the subexpression sat: -(5 - sqrt(-11)) simplified, while the same thing under an exponent, inside sgn, or as a matrix entry did not. The step came from Expand, which the simplifier offers for the root expression only and which does not descend into an exponent, a function's argument or a matrix. Written as a rule it runs wherever the shape occurs, because a rule walks the tree. A unary minus parses as (-1) * x, so the shape is (-1) * (a - b), and turning it round drops the multiplication and the negative constant together: five nodes for three, and the metric charges four more for a negative real. It goes into InvertNegativeMultipliers, whose other arm normalises 1 + (-x) into 1 - x -- the same family, and already in the bundle SimplifyChildren applies everywhere. Expand of a matrix is the same story from the other end. It reads its argument as a sum, and a matrix is not one, so a matrix left by the "too complicated, return what came in" exit and came back as it arrived. It now expands entry by entry. Factorize and Differentiate both descended already, being built out of rewrite rules, so Expand was the odd one out rather than matrices being held back on purpose. Where a caller meets it: EquationSystem.Solve returns a Matrix, so a solved system's entries were left in whatever form the solver built them in. The x2 + y system's answer goes from [[-(-(5 - sqrt(-11)) / 2 + 3), ...]] to [[-1/2 + -1/2 * sqrt(-11), ...]]. Two things in the issue's own analysis were wrong and are worth correcting, since both would mislead the next reader. The candidate for the sum case *is* generated -- `alternate` lists it second -- and it was not chosen because the two forms **tie**: both rate 22 and both have Complexity 7, and a tie is settled by whichever candidate came first. The issue's complexity table compared the input against the good candidate rather than the chosen output against it. Only the power, sgn and matrix cases are the "never generated" kind. The rule fixes all four, the sum case included, because b - a is strictly better than either and so wins the tie outright. The new tests fail 7 of 246 against master and pass 246 of 246 here. Suite 6359 passed, F# wrapper 130 passed; casbench 117/119 with 0 wrong; rootcheck 596/596; simpsweep 10463/10463; propcheck 1340 checks 0 failures; crashcheck 1652 cases 0 crashes; boundcheck unchanged at 2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #882.
-(a - b)becameb - afor a whole expression and not for the same expression inside another node,so what a caller got depended on where the subexpression sat:
-(5 - sqrt(-11))sqrt(-11) - 5-(5 - sqrt(-11)) + yy - (5 - sqrt(-11))sqrt(-11) - 5 + y2 ^ (-(5 - sqrt(-11)))2 ^ (sqrt(-11) - 5)sgn(-(5 - sqrt(-11)))sgn(sqrt(-11) - 5)[[-(5 - sqrt(-11)), 1]][[sqrt(-11) - 5, 1]]Expand([[(x+1)^2, 1]])[[(x + 1) ^ 2, 1]][[1 + 2 * x + x ^ 2, 1]]Every one of these was already the right number, written the long way round.
Why it only worked at the root
The step came from
Expand, which the simplifier offers as a candidate for the root expressiononly, and which does not descend into an exponent, a function's argument or a matrix. So it is written
as a rule instead: a unary minus parses as
(-1) * x, so the shape is(-1) * (a - b), and turning itround drops the multiplication and the negative constant together — five nodes for three, and the
complexity criteria charge four more for a negative real. It lands in
InvertNegativeMultipliers,whose other arm normalises
1 + (-x)into1 - x: the same family, and already inside the bundleSimplifyChildrenapplies everywhere. A rule walks the tree, which is the whole of the fix.Expandof a matrix is the same story from the other end. It reads its argument as a sum, and a matrixis not one, so a matrix left through the "if one is too complicated, return the current one" exit and
came back as it arrived. It now expands entry by entry.
FactorizeandDifferentiateboth descendedalready — they are built out of rewrite rules — so
Expandwas the odd one out rather than matricesbeing held back deliberately.
Where a caller meets it
EquationSystem.Solvereturns aMatrix, so a solved system's entries were left in whatever form thesolver built them in:
Two corrections to the issue's own analysis
Both would mislead whoever read it next, so they are worth stating rather than quietly fixing.
The sum case's candidate is generated.
alternate::-(5 - sqrt(-11)) + ylistssqrt(-11) - 5 + ysecond. It lost because the two forms tie:
y - (5 - sqrt(-11))andsqrt(-11) - 5 + ybothrate 22, and a tie is settled by whichever candidate was generated first — the accident the
RationaliseDenominatorcomment in that same file already warns about. Only the power,sgnandmatrix cases are the "never generated" kind the issue describes.
The complexity table in the issue compared the wrong pair. It read the input (9) against the good
candidate (7) and concluded the good form would win. The chosen output is 7 as well —
Simplifydidimprove 9 → 7, then picked one of two equally-rated 7s. And
Complexityis not what ranks candidatesin any case;
SimplifiedRateis, which weights a negative real at 4 and a negative power at 8. I haveadded a
rate::probe alongsidecomplexity::in the workspace harness so this confusion is onecommand away from being caught, with the mistake named in its comment.
The rule fixes all four cases including the sum, because
b - ais strictly better than eithercandidate and so wins the tie outright rather than depending on order.
Measured
The new tests fail 7 of 246 against
masterand pass 246 of 246 here, checked by restoringmaster's two files underneath them.
Suite 6359 passed / 0 failed, F# wrapper 130 passed. casbench 117/119 with 0 wrong, rootcheck 596/596,
simpsweep 10463/10463 — the one that matters most here, since this changes the form of a great many
expressions and simpsweep is what checks a changed form still has the old value. propcheck 1340 checks
with 0 failures, crashcheck 1652 cases with 0 crashes, boundcheck unchanged at 2 disagreements.
Cut from
masteratfbf1dd77.