Fix #340, #341: replace raw hash-equality identity checks with deep comparison - #387
Merged
Conversation
petlenz
force-pushed
the
fix-339-coeff-equality
branch
from
July 24, 2026 19:26
b51d600 to
1afce70
Compare
petlenz
force-pushed
the
fix-340-hash-identity-sweep
branch
2 times, most recently
from
July 25, 2026 11:56
349c423 to
9590ea4
Compare
…omparison The intentional hash-collision design (hash(c*T)==hash(T), hash(pow(T,c))==hash(T), coefficient-blind n-ary hashes) is only safe when every consumer that buckets by hash follows up with a deep comparison. Several simplifier sites used raw hash_value() equality as semantic identity, silently mis-merging colliding expressions: X+pow(X,2) -> 2*pow(X,2), X+2*X -> 4*X, X*(2*X) -> pow(X,2) with the 2 dropped, sin(x+2)-sin(x+5) -> 0, max(2x,3x) -> 2x, and the projector addition rules merging vol(pow(X,2))+dev(X) -> sym(pow(X,2)) (#341). Swept sites: - core sub_dispatch::get_default zero fold -> operator== - scalar max()/min() identity fold -> operator== - scalar/tensor differentiation leaf match -> operator== - tensor add_default::get_default, dispatch(tensor_negative), tensor mul get_default pow fold -> operator== - tensor_scalar_mul_add / add_negative dispatchers -> deep compare of the inner tensor operand - symbol_add pointer-identity check -> operator== - add_base projector addition rule and tensor_projector_simplifier grouping -> deep argument equality after the hash bucket (#341) Like-term merging is preserved and extended via the #339 infrastructure: tensor_scalar_mul overrides like_term_of (c*T is a like term of T and of c'*T), n_ary_add::dispatch uses find_like, and add_default::get_default folds (c1*T)+(c2*T) -> (c1+c2)*T through a .cpp-defined helper (operators are not visible in the header TU). (X+Y)+2*X now merges to 3*X+Y for the first time; the AddNaryPlusScalarMul expectations documenting the old no-merge limitation are updated accordingly. tensor_add's running space join is bypassed by merge-based insertion paths (merge_or_insert does not go through the push_back override); recompute_space() re-derives the join from the final children and is called on those paths, fixing sym(A)+sym(B) losing its Symmetric annotation and covering the previously-latent gap in the add+add merge_add path. Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
Mirror of the #339 review fixes on the tensor side: the n_ary_add like-term merge now filters zero results, invalidates the mutated copy's cached hash, recomputes the space join, collapses empty/single-child results, and probes the inner tensor when the incoming term is c*T with the bare T stored ((X+Y) + 2*X -> 3*X+Y). The negative-cancellation path gets the same invalidate/recompute/ collapse treatment. Regression test included. Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
petlenz
force-pushed
the
fix-340-hash-identity-sweep
branch
from
July 25, 2026 13:25
9590ea4 to
f2a2102
Compare
…pow parity, fraction printing Round 5 of the stacked-PR review (whole-diff cross-check plus a fresh differential fuzzer, 20k seeds) surfaced eleven defects. All land here because they are missed sites of this branch's theme: identity decisions made on raw hashes (or hash-keyed map lookups) instead of deep equality, plus the value-corruption bugs the property sweep pinned. Simplifier identity/merge fixes: - constant_add/one_add dispatch(add): the coefficient-cancel path returned the original rhs with its old coefficient intact, silently dropping the lhs constant (sweep bug A: 5 + (x - 5) -> x - 5). - n_ary_add dispatch(negative): erase-by-key confirmed only by the hash-based comparator; wrapper(3+a) aliases wrapper(1+a), so the wrong child was erased and both constants vanished (sweep bug C). Cancelling now requires deep equality, and add + (-add) reroutes through subtraction, which cancels childwise. - negative_add dispatch(add): (-x) + (y - x) hit the no-duplicates assert; now merge_or_insert + finalize_add. - mul_sub/symbol_sub/mul_add/symbol_add like-term folds: raw hash equality both aliased across different children (x vs x+2) and, for symbol-vs-mul, never fired at all (n-ary hashes are id-tagged); all four sites now use an explicit single-child deep comparison. The sub fold also classified the sign with rank-lexicographic operator< so negative double differences came out positive (sweep bug B / R5-2); now numeric_less. - sub_dispatch::get_default: z - (-z) pushed z twice (assert); now merge_or_insert + finalize_add. - generic mul dispatch(mul) and scalar n_ary_mul fallback: a duplicate factor (sin(x) * (c*sin(x)*y)) hit the assert; both now combine the existing factor into a pow. - tensor pow/mul folds compared raw hashes; hash(c*T)==hash(T) and hash(pow(T,c))==hash(T) folded A*pow(2A,2) into pow(A,3). All three sites deep-compare the pow bases now. - pow dispatch: pow(-b, p) extracted the sign for every exponent; (-b)^2 = b^2, and for symbolic/non-integer p no extraction is valid. Extraction is now gated on integer parity. Two adjacent division shortcuts also erased map children via hash-keyed lookups without a deep check and left stale cached hashes. Printer fixes (R5-5): - partition_mul_fractions classified negative double exponents with the rank-lexicographic operator<, so pow(x,-2.5) never reached the denominator; now numeric_less. - scalar_mul printer emitted a trailing '*' before '/' when the numerator was empty, glued multi-factor denominators together without separators or parens, and omitted the '1' numerator. The t2s printer had the same missing-'1' hole. Tests: two pinned expectations in ScalarExpressionTest/ TensorToScalarExpressionTest asserted the incorrect pow(-x,2) -> -pow(x,2) extraction and were corrected; regression tests for every finding added to CoreBugFixTest (RoundFiveReview suite). Refs #340. Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
…rrors, t2s sub coeff fold Round 6 of the stacked-PR review (adversarial probe of the round-5 delta) confirmed three findings, all mirrors or gaps of the round-5 fixes: - Symbolic t2s add coefficients were silently deleted: every get_coefficient + coeff().free() fold site reported 0 for a valid-but-non-numeric coefficient (e.g. wrapper(s) set by constant_sub_dispatch) and then destroyed it, so (w(s) - (tr(A)+det(A))) + w(5) dropped s entirely. All eight fold sites now route through a shared fold_constant_into_add_coeff helper (simplifier_common.h) that combines a symbolic coefficient as an expression instead of folding numerically. - The value==0 single-child collapse round 5 added to constant+add / one+add was missing from the six mirror sites (add±constant, add±one, add+(-numeric), -numeric+add): (x+5)-5 stayed a degenerate single-child add that printed "x" but compared unequal to x, breaking downstream cancellations like x-((x+5)-5) -> 0. The shared helper finishes through finalize_add at every site. - The t2s sub-side wrapper merge still pushed a numeric merged wrapper as a child; the add side was fixed in round 5 to fold it into the coefficient. (tr(A)+w(a+3))-w(a+1) and tr(A)+w(2) now compare equal and (tr(A)+w(a+3))-w(a+1)-w(2) cancels to tr(A). Also: invalidate_hash() hygiene after every raw symbol_map().erase in scalar_simplifier_mul.cpp (the round-5 bug class; flagged plausible in round 6 — all current continuations re-invalidate, this makes it unconditional). Regression tests in CoreBugFixTest (RoundSixReview suite). Refs #340. Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
…tion-pair cancellation Round 7 (adversarial probe of the round-6 delta) confirmed four findings; three are fixed here, the fourth is tracked as an issue. - Negative-lhs subtraction built raw negative nodes: -e1 - e2 went through make_expression<negative>(e1+e2) with no normalization, so a fully-cancelling sum minted negative(zero) — (-x)-(-x) printed -0 and defeated every is_same<zero_type> filter downstream ((z-y)-(x-y) produced "z-0-x"). Both domains' sub_base dispatch(negative) and dispatch(zero) now route through operator-, which normalizes -0, -(-x), and (t2s) negative wrappers. - -w(a) and w(-a) are the same value but compared unequal, and round-trip cancellation ((base - w(a)) + w(a)) failed because the wrapper scan cannot see negative children. t2s neg_fn now normalizes -wrapper(s) to wrapper(-s) globally. - Negation pairs share no hash, so add merges never cancelled a child against its exact negation: (3-x)+x stayed "3+x-x" and (3+x)+(1-x) stayed unreduced (while the mirrored negative-rhs forms worked). find_like now retries with the exact negation in n_ary_add_dispatch::dispatch(SymbolType) and merge_add; merge_add gained a zero-filter predicate and its dispatch site finishes through finalize_add. Matches are restricted to exact negations — like-terms of -x would combine into nested adds. Not fixed here (needs a design decision, tracked separately): symbolic t2s wrapper values have two canonical homes (sub-built expressions put them in the add coefficient, add-built ones push them as children), so print-equal expressions compare unequal and coeff-vs-child wrapper pairs never merge. Regression tests in CoreBugFixTest (RoundSevenReview suite). Refs #340. Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
…ression
Round 8 (adversarial probe of the round-7 delta) confirmed two
regressions introduced by the round-7 negation probe in merge_add:
- Double-consume (wrong value): when the lhs held an exact {t, -t}
pair, t consumed the rhs child -t via the negation probe and then -t
direct-matched the same rhs child again — (5x-5x) + (y-5x) evaluated
to y-10x. Matches are now gated on the used-set in both probe paths.
The enabler is closed too: child insertion in merge_add and
merge_and_finish now goes through add_insert_signed, which combines
an entry with an existing exact match or exact negation until no
collision remains and zero-filters the result, so an add can no
longer hold an exact {t, -t} pair at all ((2x-5x)+3x now collapses
to the zero singleton at construction).
- Tensor zero child: the shared merge_add gained cancellation power in
round 7, but only the scalar/t2s dispatch site got the zero filter —
the tensor caller inserted fully-cancelled combines as literal 0{2}
children ((A+B)+(C-A) -> "0{2}+B+C", breaking identity and
round-trip cancellation). The tensor site now passes the
is_same<tensor_zero> filter and collapses degenerate results the
same way the scalar path does; the filter-less merge_add overload is
gone so no caller can opt out silently.
Regression tests in CoreBugFixTest (RoundEightReview suite).
Refs #340.
Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
…onverted tensor insert, zero coeff
Round 9 (adversarial probe of the round-8 delta) confirmed three
value-preserving structural defects:
- Nested adds: add + (-t) for a non-cancelling t, and add - expr in
both the core and tensor sub get_default, pushed the WHOLE lhs add
as a single child. (C-A)-B built tensor_add{-B, tensor_add{C,-A}},
and buried children then defeated merge cancellation:
(A+B)+((C-A)-B) kept an unreachable interior -A ('A+C-A'). The
negative dispatch and both get_defaults now copy the add and
signed-insert the other operand, finishing through the usual
collapse. A tensor print expectation that had pinned the nested
shape's output ordering was updated to the flat form.
- The tensor n-ary template dispatch still inserted its combined term
via plain merge_or_insert: (5A-2A)+(-3A) held an exact {2A,-(2A)}
pair instead of collapsing to zero. Now add_insert_signed, matching
the scalar/t2s merge_and_finish.
- merge_add stored a cancelled coefficient as a literal zero holder:
(2+x)+(y-2) printed '0+x+y' and compared unequal to x+y. Cancelled
coefficients are now dropped.
Also converted every remaining dispatcher-side merge_or_insert in
simplifier_sub.h/simplifier_add.h to the shared insert_signed helper
(exact-negation combining + zero filter) so no insertion path can
leave a {t,-t} pair or a zero child.
Regression tests in CoreBugFixTest (RoundNineReview suite).
Refs #340.
Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
…cancel
The tensor n_ary_add template dispatch probed find_like and the
scalar-mul reverse probe, but its miss path was a raw push_back — the
one insertion path left without the exact-negation probe, so
((A+B)-C)+C kept a {C,-C} pair (value-correct, identity-broken).
The miss path now routes through add_insert_signed with the
tensor_zero filter and the usual collapse, mirroring the scalar/t2s
SymbolType dispatch.
Regression tests in CoreBugFixTest (RoundTenReview suite).
Refs #340.
Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
The generic swap-to-n-ary dispatch for non-add + add was guarded on a non-void mul_type, and the tensor add_default's own catch-all shadowed it anyway — so tensor A+(B+C) fell to get_default, which pushed the rhs add wholesale as a single child. A+(B+C) and (A+B)+C were different trees (equal values, unequal identity; like-terms buried in the nested child never merged). add_default now swaps so the add becomes the LHS (n_ary_add overrides the same dispatch with the real merge, so there is no swap loop), and the core guard is dropped — any domain reaching that dispatch has an add-lhs route. Regression tests in CoreBugFixTest (RoundElevenReview suite). Refs #340. Signed-off-by: petlenz <peterlenz89.pl@gmail.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
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 #340, fixes #341. Stacked on #386 (#339) — the deep
operator==this sweep relies on lands there.Every simplifier site that used raw
hash_value()equality as semantic identity now deep-compares. Probe-confirmed wrong results fixed:X + pow(X,2)→2*pow(X,2)X+pow(X,2)X + 2*X→4*X3*XX * (2*X)→pow(X,2)(2 dropped)X*2*X(value-correct)vol(pow(X,2)) + dev(X)→sym(pow(X,2))sin(x+2) - sin(x+5)→0max(2x, 3x)→2x(X+Y) + 2*X→X+2*X+Y(unmerged)3*X+Y(new merge)Notes
tensor_scalar_muloverrideslike_term_of(c·T ~ T ~ c'·T), which plugs the tensor domain into n_ary_tree operator== ignores the coefficient — x+2 == x+5 and 2*x == 3*x compare equal, corrupting simplify/compare/substitute #339'sfind_likemachinery — the like-term merges the shared-hash design exists for now work in both argument orders.(c1*T)+(c2*T)fold is defined in the .cpp (operators not visible in the header TU, per the established pattern).tensor_add's running space join is bypassed by merge-based insertion (merge_or_insertdoesn't go through thepush_backoverride); newrecompute_space()re-derives it, fixingsym(A)+sym(B)losing its Symmetric tag and a latent gap in the add+add merge path.TensorExpressionTest.AddNaryPlusScalarMulpinned the documented no-merge limitation ('find() doesn't match existing bare symbols'); its expectations update to the merged forms mandated by Raw hash_value() equality used as identity in simplifiers — X+pow(X,2)→2*pow(X,2), X+2*X→4*X, X*(2*X) drops the 2, sin(a)−sin(b)→0 #340.Testing
5 new
HashIdentitySweeplock-in tests (tensor add/mul tables, projector argument equality, scalar sub/max/min, space-join survival). Full suite: 2426/2426 pass.