Fix #339: n_ary equality must compare the coefficient - #386
Merged
Conversation
n_ary_tree/n_ary_vector operator== ignored m_coeff, so x+2 == x+5 and 2*x == 3*x compared equal, and the false identity propagated through unary/binary node equality (sin(x+2) == sin(x+5)). Simplifier guards, comparison folds, if_then_else branch collapse, and substitute all treated this as semantic identity, producing wrong results such as (x+2)*(x+5) -> pow(x+2,2) and (x+2)+(-(x+5)) -> 0. The coefficient-blind hash is kept: it is the like-term map-keying design. What changes: - operator== compares coefficients (invalid == absent). - operator< gets a coefficient tiebreak after the childwise loop, so x+2 and x+5 are distinct map keys and can coexist as children. - The like-term relation becomes explicit: expression::like_term_of (equal up to the n-ary coefficient), overridden by n_ary_tree, with find_like scanning the equal-hash run. merge_or_insert and the add dispatchers use it, so 2x+3x -> 5x and ((x+y)+x)+x -> 3x+y keep merging; the symbol dispatch retries with a bare mul probe since c*x hashes into a different run than x (fixes #347's merge miss). - get_default folds (c1*T)+(c2*T) -> (c1+c2)*T with coefficients combined as expressions (symbolic t2s coefficients survive), collapsing to T on sum 1 and zero on sum 0; the mul+mul add dispatch routes through the same fold instead of a raw hash check. - The scalar pretty-printer comparator gets a full-comparison tiebreak so distinct children are never deduped away (fixes the ((x+y)+x)+x printing as 2*x+y while evaluating 3x+y, #347). Lock-in tests in CoreBugFixTest.h cover the equality table, the no-false-fold table, printer child retention, preserved like-term merges, and the sum==1/sum==0 collapses. Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
petlenz
force-pushed
the
fix-339-coeff-equality
branch
from
July 24, 2026 19:26
b51d600 to
1afce70
Compare
…obe, stale hashes Adversarial review of PR #386 found four defects in the new identity machinery: - nary_coeff_less used expression < directly, which orders by promotion rank first (int 2 before double 2.0) while nary_coeff_equal is value-based - two adds could satisfy a == b and a < b simultaneously, so sub-side map lookups missed and ((x+y)+x) - (2.0*x+y) stayed '2*x-2*x'. The tiebreak now short-circuits on equality. - A like-term cancellation could insert a literal zero child ((2*tr(A)+tr(B)) + (-2)*tr(A) -> '0+tr(B)'): the merge paths now filter zero results and collapse empty/single-child adds (merge_and_finish in the generic dispatcher; a local finish lambda in the t2s template, which lives in the .cpp for operator visibility). - The like-term probe was one-directional (stored c*x found for incoming x, but stored x missed for incoming c*x); the catch-all dispatch now probes the mul's bare child in the reverse direction, scalar and t2s. - Mutating a copied n-ary node kept the source's cached hash, so == fast-rejected structurally equal twins and sin(a+x) - sin(fresh) failed to cancel. insert_hash now invalidates the cache, and the erase-only cancellation paths call the new invalidate_hash(). Four regression tests lock each finding in. Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
Round-2 adversarial review confirmed all round-1 fixes correct but found the invalidate/zero-filter/collapse sweep incomplete: - finalize_add now invalidates the mutated copy's cached hash for every caller (the sub cancel branch filtered zeros but kept the 3-child hash: a=x+y+z hashed, a-x compared unequal to y+z). - The trig-Pythagorean branch collapses and invalidates (cos2+y hashed, +sin2 -> 1+y now equals y+1; coefficient-cancel no longer leaves a bare single-child add). - n_ary_add::dispatch(scalar_pow)'s non-Pythagorean fallback routed a bare push_back around the fixed catch-all: (pow(x,2)+y)+pow(x,2) threw 'duplicate child insertion'. It now defers to the catch-all. - The t2s wrapper-cancel overloads (add and sub) collapse and invalidate; a cancelled wrapper left an uncollapsed single-child add that compared unequal to its own child. - n_ary_mul_sub_dispatch treated an invalid mul coefficient as 0: ((x*y)*pow(x,-1)) - y evaluated to -3 instead of 0. The default for a mul is 1. - The t2s .cpp catch-all templates were proven dead by disassembly (the core constrained templates win overload resolution; correctness comes from the core merge_and_finish) - deleted, closing #370. The live scalar_wrapper overloads keep their domain-specific handling. Five regression tests lock the findings. Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
This was referenced Jul 26, 2026
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
1 participant
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.
Summary
Fixes #339, fixes #347.
n_ary_tree/n_ary_vectoroperator==ignored the coefficient, sox+2 == x+5and2*x == 3*xcompared equal — and the false identity propagated throughunary_op/binary_opequality into every simplifier guard, comparison fold,if_then_elsecollapse, andsubstitute. Probe-confirmed wrong results fixed by this PR:(x+2)*(x+5)→pow(x+2,2)(evaluated 9 for 18)(2+x)*(5+x), evaluates 18(x+2)+(-(x+5))→0eq(2x,3x)→1substitute(x+5, x+2, y)→y((x+y)+x)+xprinted2*x+y, evaluated 133*x+y(#347)Design
The coefficient-blind hash is kept — it is the like-term map-keying design. The compensating pieces:
operator==compares coefficients;operator<gets a coefficient tiebreak, so coefficient-differing siblings can coexist as map keys.expression::like_term_of, overridden byn_ary_tree) withfind_likescanning the equal-hash run;merge_or_insert,merge_add, and the add dispatchers use it, so2x+3x → 5xand(y+2x)+3x → 5x+ykeep merging. The symbol dispatch retries with a baremul{x}probe (c*xhashes into a different run thanx) — this makes((x+y)+x)+x → 3*x+ymerge for the first time.get_defaultfolds(c1*T)+(c2*T) → (c1+c2)*Twith coefficients combined as expressions (symbolic t2s coefficients survive; sum 1 collapses to T, sum 0 to zero). The mul+mul add dispatch routes through the same fold instead of its raw-hash check.2*x+y-for-13 bug).Testing
CoreBugFixTest.h(equality table, no-false-fold table incl. evaluator cross-checks, printer child retention, preserved like-term merges, sum==1/0 collapses).Remaining raw-
hash_value()identity checks (tensor add/mul dispatchers, sub fold, max/min, diff leaf match) are #340 and land on top of this branch.