Repository navigation
Fix three wrong answers in the solvers and evaluation (#632, #442, #662) - #664
Merged
Happypig375 merged 7 commits intoAug 3, 2026
Merged
Conversation
Happypig375
reviewed
Aug 3, 2026
| { | ||
| // Careful: this node is declared Minusf(Subtrahend, Minuend), so the | ||
| // expression is `Subtrahend - Minuend` -- the two property names are the | ||
| // reverse of the usual terminology. Reading them as their names suggest is |
Member
There was a problem hiding this comment.
@Rafael-SOWNet I suggest fixing this to their usual terminology instead
Happypig375
requested changes
Aug 3, 2026
Rafael-SOWNet
force-pushed
the
fix/evaluation-and-solvers
branch
from
August 3, 2026 17:52
c94657a to
6bf23bc
Compare
Member
|
@Rafael-SOWNet Can you also submit a PR to fix the CI while you're here? Thanks |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #664 +/- ##
==========================================
- Coverage 80.99% 79.95% -1.05%
==========================================
Files 155 155
Lines 13687 12512 -1175
Branches 1957 2032 +75
==========================================
- Hits 11086 10004 -1082
+ Misses 1990 1917 -73
+ Partials 611 591 -20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`sqrt(i) = (1 + i) / sqrt(2)` simplified to False. Both sides are the same number, but Equalsf compared their separately evaluated values for exact digit equality, and evaluating each rounds independently -- they came out as ...863 + ...864i versus ...861 + ...861i, differing in the last ulps. Fall back to testing whether the difference is zero. The difference cancels, and Real's factory already maps anything below PrecisionErrorZeroRange onto an exact zero, so this reuses the library's existing notion of zero rather than introducing a new tolerance. The exact-equality fast path is kept, so infinities keep comparing as before. Also fixes `sqrt(2) * sqrt(3) = sqrt(6)` and `sin(pi/4) = sqrt(2)/2`. Full suite: 3688 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Solving `a = 1 / (b - c)` for c returned `1/a - b`, the negation of the correct `b - 1/a`. Concretely, `2 = 1/(5 - c)` answered -4.5 where the answer is 4.5, which is what the reporter saw as "the value is negative". Minusf is declared `Minusf(Subtrahend, Minuend)`, so the expression is `Subtrahend - Minuend` and the two property names are the reverse of the usual terminology. Read as their names suggest, the branch for `a - x = value` returned `value - a` instead of `a - value`. Only that branch was wrong; the other one was already consistent with the actual declaration order. Added a comment recording the naming trap. Full suite: 3694 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`MathS.FromString("(|[12,15] - [0,0]|)").EvalNumerical()` threw
CannotEvalException on 1.4.0 where 1.3.0 returned a value, while
`(|[2,1] - [1,1]|)` worked -- the difference being whether the norm came
out a perfect square.
Absf's vector branch built the Euclidean norm and always finished with
InnerSimplified, ignoring the isExact flag its sibling branches respect.
sqrt(369) has no exact simplification, so it survived as a Powf and
EvalNumerical rejected it as not a simple number. Now the exact path
still returns sqrt(369) and the numeric path evaluates it.
Full suite: 3701 passed, 0 failed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`lim x->0 (1+x)^(1/x)` answered 1; the answer is e. The rewrite of f(x)^g(x) into e^(g(x)*(f(x)-1)) required g to have an infinite two-sided limit, but at x -> 0 the exponent 1/x goes to -oo on the left and +oo on the right, so no two-sided limit exists even though the magnitude diverges -- which is all the rule needs. The rewrite itself only uses the product g(x)*(f(x)-1), which is fine from either side. Now also accepts the case where both one-sided limits diverge. The x -> +oo form already worked and is pinned by a test so it stays working. Found by the solver-coverage corpus, not reported upstream. Full suite: 3706 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two wrong answers, one root cause each, both in the path from an equation to its roots. Neither is in the tracker; the solver corpus found them. `ln(x) + ln(x+1) = 0` returned two roots. Solving it goes through log(a) + log(b) = log(a * b) and then through x^2 + x - 1 = 0, whose roots are 0.618... and -1.618... The second is not a root of the original: both logarithms are taken off the negative reals there, and the left-hand side comes to 2*pi*i. The rewrites that widen a domain this way cannot each carry a condition that survives the chain of substitutions after them, so the answers are checked once, at the top, against the equation as the caller wrote it. A root is dropped only on positive evidence -- anything that will not evaluate to a number, a parametric family like pi + 2*pi*n_1 among them, is kept. `x + ln(x) = 0` threw UncompilableNodeException out through the public Solve. The Newton solver compiles the simplified expression, simplification leaves Providedf nodes behind, and the compiler has no way to represent one. The conditions now come off before compilation and the roots are verified after, which is where they belonged anyway. Measured on the 117-problem solver corpus: 78 -> 80 solved, and both the wrong answer and the error go to zero. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One test per issue, named for it, plus the two wrong answers that had no issue filed, so that a future refactor that reinstates any of these fails loudly.
In `a - b`, `a` is the minuend and `b` the subtrahend. The record had them the other
way about -- `Minusf(Entity Subtrahend, Entity Minuend)` -- so the first operand, the
one on the left of the sign, was called the subtrahend.
That is what caused the sign error this branch fixes. The inverter was written to
what the names say rather than to which operand is which, so `a = 1/(b - c)` solved
to the negation of the right answer. The previous commit fixed the inversion and left
a comment warning about the names; naming them correctly is better, and removes the
need for the warning.
The swap is mechanical -- every use is positional, so each name simply exchanges with
the other across ten files, and the compiler's operand order is unchanged because the
objects are the same. The InvertNode body now reads as it means:
Minuend.ContainsNode(x)
? Minuend.Invert(value + Subtrahend, x) // x - a = value => x = value + a
: Subtrahend.Invert(Minuend - value, x); // a - x = value => x = a - value
These are public property names, so anyone reading them by name gets the other
operand than before. That is the point: they were wrong, and code written against
them was wrong with them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rafael-SOWNet
force-pushed
the
fix/evaluation-and-solvers
branch
from
August 3, 2026 19:02
6bf23bc to
2ab53e5
Compare
Member
Author
|
Done — the names are swapped rather than worked around, and rebased onto current master.
The inverter reads as it means now, and the comment warning about the naming is gone with it: Minuend.ContainsNode(x)
? Minuend.Invert(value + Subtrahend, x) // x - a = value => x = value + a
: Subtrahend.Invert(Minuend - value, x); // a - x = value => x = a - valueWorth flagging since these are public property names: anyone reading them by name now gets the other operand than before. That is rather the point — they were the wrong way round, and code written against them was wrong with them. This is what caused the sign error in the first place. 3862 unit tests and 127 F# tests pass on the rebase, and both target frameworks build. |
There was a problem hiding this comment.
Pull request overview
This PR fixes several correctness issues across evaluation, limits, and equation solving in AngouriMath, and adds a new regression test suite to prevent the reported wrong answers and exceptions from recurring.
Changes:
- Fix subtraction operand handling (
Minusf) across substitution, formatting, evaluation, differentiation, limits, compilation, and solver inversion. - Make constant equality simplification robust to independent rounding by comparing evaluated differences against the library’s zero tolerance.
- Fix numeric evaluation of vector norms, apply the second remarkable limit when the exponent diverges in magnitude, and verify solver roots against the original equation to drop demonstrably spurious roots.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/Tests/UnitTests/Common/SolverRegressionTest.cs | Adds regression tests covering constant equality, subtraction inversion, vector norms, remarkable limits, root verification, and Newton-solver exception leakage. |
| Sources/AngouriMath/Functions/Substitute.cs | Aligns Minusf.Substitute child order with updated Minusf semantics. |
| Sources/AngouriMath/Functions/Output/ToSympy/ToSympy.Arithmetics.Classes.cs | Fixes SymPy output ordering for subtraction. |
| Sources/AngouriMath/Functions/Output/ToString/ToString.Arithmetics.Classes.cs | Fixes string output ordering for subtraction. |
| Sources/AngouriMath/Functions/Output/Latex/Latex.Arithmetics.Classes.cs | Fixes LaTeX output ordering for subtraction. |
| Sources/AngouriMath/Functions/Evaluation/Evaluation.Discrete/Evaluation.Discrete.Classes.cs | Updates constant equality simplification to compare evaluated differences using Number.IsZero. |
| Sources/AngouriMath/Functions/Evaluation/Evaluation.Continuous/Evaluation.Continuous.Arithmetics.Classes.cs | Fixes subtraction simplification argument order; updates vector norm handling to respect numeric vs exact mode. |
| Sources/AngouriMath/Functions/Continuous/Solvers/NumericalSolving/NewtonSolver.cs | Strips Providedf conditions before compilation and verifies rounded candidate roots against the original expression. |
| Sources/AngouriMath/Functions/Continuous/Solvers/EquationSolver/SolveStatement.cs | Filters finite root sets by substituting back into the original equation and dropping demonstrably spurious roots. |
| Sources/AngouriMath/Functions/Continuous/Solvers/EquationSolver/InvertNode.Classes.cs | Fixes subtraction inversion logic/sign when isolating a variable in a subtraction. |
| Sources/AngouriMath/Functions/Continuous/Limits/Transformations.cs | Updates second remarkable limit trigger to accept one-sided divergence in magnitude of the exponent. |
| Sources/AngouriMath/Functions/Continuous/Limits/Solvers/Limit.Classes.cs | Fixes operand order when rebuilding Minusf during limit computation. |
| Sources/AngouriMath/Functions/Continuous/Differentiation.cs | Fixes derivative rule application order for subtraction. |
| Sources/AngouriMath/Functions/Compilation/IntoFE/Compiler.cs | Adjusts compilation operand push order to match CALL_MINUS VM evaluation order. |
| Sources/AngouriMath/Core/Entity/Continuous/Entity.Continuous.Operators.Classes.cs | Changes Minusf record parameter naming/ordering and updates linearization logic accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+70
| private static bool DivergesInMagnitude(Entity power, Variable x, Entity dest) | ||
| => IsInfiniteNode(power.Limit(x, dest)) | ||
| || (IsInfiniteNode(power.Limit(x, dest, ApproachFrom.Left)) | ||
| && IsInfiniteNode(power.Limit(x, dest, ApproachFrom.Right))); |
Comment on lines
+8
to
+11
| using AngouriMath; | ||
| using AngouriMath.Extensions; | ||
| using PeterO.Numbers; | ||
| using Xunit; |
Comment on lines
60
to
+67
| /// <summary> | ||
| /// A node of difference | ||
| /// </summary> | ||
| public sealed partial record Minusf(Entity Subtrahend, Entity Minuend) : ContinuousNode, IBinaryNode | ||
| public sealed partial record Minusf(Entity Minuend, Entity Subtrahend) : ContinuousNode, IBinaryNode | ||
| { | ||
| /// <summary>Reuse the cache by returning the same object if possible</summary> | ||
| private Minusf New(Entity subtrahend, Entity minuend) => | ||
| ReferenceEquals(Subtrahend, subtrahend) && ReferenceEquals(Minuend, minuend) ? this : new(subtrahend, minuend); | ||
| private Minusf New(Entity minuend, Entity subtrahend) => | ||
| ReferenceEquals(Minuend, minuend) && ReferenceEquals(Subtrahend, subtrahend) ? this : new(minuend, subtrahend); |
Happypig375
approved these changes
Aug 3, 2026
Happypig375
pushed a commit
that referenced
this pull request
Aug 4, 2026
Both were left by the Copilot reviewer on #664, which merged before I had read them. SolverRegressionTest had `using PeterO.Numbers;` and never named a type from it -- the EDecimal occurrences are all property accesses on a Real, which need no using. Removed. The library builds with TreatWarningsAsErrors, so an unused using is worth not leaving lying about. DivergesInMagnitude was flagged for calling Limit up to three times. The three are distinct and the || short-circuits, so there was nothing to cache, but there was a real waste next to it: at an infinite destination there is only one side to approach from, so the two one-sided limits are both pointless and meaningless there. They are now only asked for when the destination is finite. Co-authored-by: Claude Opus 5 <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.
Five fixes to evaluation, limits and the equation solvers. Three of them are wrong
answers rather than unsimplified ones. Each has a regression test, and each was
reproduced against a stock
masterbuild before being claimed.#632 —
a = 1/(b - c)solved to the negation of the right answerOpen since April 2024.
Minusf's inverter had its two branches the wrong way roundwith respect to its own comments. The trap is in the record declaration:
The names are reversed from standard terminology — the expression is
Subtrahend - Minuend. The branch was written to the names rather than to thepositions, so a subtraction containing the unknown inverted with the wrong sign. Only
the second branch is wrong; my first attempt swapped both and produced a different
wrong answer, which the numeric assertions in the new test caught.
#442 —
sqrt(i) = (1 + i)/sqrt(2)answeredFalseEquality of two constants compared their separately evaluated values for exact digit
equality. Those two are the same number, but evaluating each rounds it differently in
the last few digits, so the comparison said they differ. It is now decided on the
difference. The test asserts both directions, so that the fix cannot collapse genuinely
different constants into equality.
#662 —
EvalNumericalthrew on some vectorsA 1.3 → 1.4 regression. The Euclidean norm of a vector was built and then
InnerSimplifiedeven when the caller had asked for numeric evaluation, sosqrt(369)survived as a
PowfandEvalNumericalrefused it. It only worked when the normhappened to be a perfect square. The branch now honours
isExactlike its siblings.lim x→0 (1 + x)^(1/x)returned1instead ofeNot in the tracker; found by a solver corpus I ran, not by reading code. The second
remarkable limit required the exponent's two-sided limit to be infinite, but
1/xat 0tends to −∞ from the left and +∞ from the right, so the two-sided limit does not exist
even though the magnitude diverges. Both one-sided limits diverging is enough, because
the rewrite only uses the product
g(x)·(f(x) − 1), which is well defined from eitherside.
ln(x) + ln(x+1) = 0returned an extraneous root, andx + ln(x) = 0threwAlso not in the tracker.
Solving the first goes through
log(a) + log(b) = log(a·b)and thenx² + x − 1 = 0,whose roots are 0.618... and −1.618... The second is not a root of the original: both
logarithms are taken off the negative reals there, and the left-hand side comes to
2πi. The rewrites that widen a domain this way cannot each carry a condition thatsurvives the chain of substitutions after them, so the answers are now checked once, at
the top, against the equation as the caller wrote it. A root is dropped only on
positive evidence — anything that will not evaluate to a number, a parametric family
like
pi + 2*pi*n_1among them, is kept.The second leaked an internal
UncompilableNodeExceptionout through the publicSolve. The Newton solver compiles the simplified expression, simplification leavesProvidedfnodes behind, and the compiler has no way to represent one. The conditionsnow come off before compilation and the roots are verified after, which is where they
belonged anyway.
I also tried the more obvious fix for the extraneous root — attaching
provided a > 0 and b > 0to the log-sum rewrite. It works, but it makes every log sumprint a condition and downgrades the exact
(-1 + sqrt(5))/2to a decimal, so Ireverted it. Verifying roots fixes the same bug, covers the whole class of
domain-widening rewrites rather than one instance, and changes no other output. The
rewrite is still formally unsound and I would rather that were tracked separately than
smuggled in here.
Testing
UnitTests3697 passed / 0 failed,FSharpWrapperUnitTests127 passed / 0 failed. Noexisting test needed changing.
This is one of three independent PRs; the other two cover parsing and precision, and
the simplifier. They touch disjoint files and can be merged in any order.
🤖 Generated with Claude Code