Drop un-combined zero coefficients when collecting like terms#167
Drop un-combined zero coefficients when collecting like terms#167oameye wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
==========================================
+ Coverage 94.44% 94.48% +0.03%
==========================================
Files 23 23
Lines 2090 2105 +15
==========================================
+ Hits 1974 1989 +15
Misses 116 116 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
050dd17 to
fc47c5e
Compare
`_addto_key!` collects like terms and deletes a key when the merged coefficient is zero, using an exact-negation test (`_isneg_cnum`) for the symbolic prefactors Symbolics leaves un-combined (e.g. `γ/D + (-γ)/D`). A sign flip, however, rewrites `λ/2` (a `/` node) as the rational `(-1//2)·λ` (a `*` node), so the negated and un-negated halves compare unequal and a mathematically-zero term survives. For a quadratic Hamiltonian `H = Δ·a†a + (λ/2)(a†² + a²)`, `commutator(im*H, a*a)` kept spurious fourth-order operators (`a†a†aa`, `aaaa`) with coefficient `λ/2 - (1//2)λ`. The negation test now canonicalizes division-by-an-integer-constant to the rational form before comparing, so the two representations cancel. It stays O(1) for the common non-fraction prefactors (a guard skips the re-compare when neither side canonicalized), so nested-commutator performance is unchanged and stored coefficients keep their original display. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fc47c5e to
bd9766b
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a symbolic-prefactor cancellation edge case in like-term collection so mathematically zero terms don’t remain in QAdd when the two cancelling coefficients are equal but represented with different Symbolics expression trees (e.g. λ/2 vs (1//2)*λ), which previously caused spurious higher-order operator terms to persist after merges (notably in commutators).
Changes:
- Improve the exact-negation cancellation check by canonicalizing
p / n(integer-literal division) to(1//n) * pbeforeisequalcomparison. - Add a regression test covering the
λ/2vs(1//2)λcommutator scenario to ensure no spurious 4th-order terms survive. - Document the fix in the changelog under Unreleased.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/arithmetics/commutator_test.jl | Adds a regression test ensuring differently-represented zero coefficients cancel and don’t leave higher-order operator terms. |
| src/expressions/cnum.jl | Updates symbolic negation detection with a lightweight canonicalization for division-by-integer cases to enable cancellation. |
| Changelog.md | Adds an Unreleased entry describing the cancellation fix and its impact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -52,7 +52,36 @@ end | |||
|
|
|||
| # Structural `a == -b`; see `_addto_key!` for why this is needed. | |||
|
|
||
| ### Fixed | ||
|
|
||
| - Like-term collection no longer keeps zero coefficients that differ only in representation. A sign flip turns `λ/2` (a `/` node) into the rational `(-1//2)·λ` (a `*` node), so the exact-negation test in `_addto_key!` saw two different trees and the term survived. For example `commutator(im*H, a*a)` with a quadratic (squeezing) Hamiltonian `H = Δ·a†a + (λ/2)(a†² + a²)` kept spurious fourth-order operators (`a†a†aa`, `aaaa`) with a zero-but-un-combined coefficient `λ/2 - (1//2)λ`, which downstream `average`/cumulant code then had to special-case. The negation test now canonicalizes division-by-an-integer-constant to the rational form before comparing, so the two representations cancel. The check is O(1) for the common non-fraction prefactors, so nested-commutator performance is unaffected ([#167](https://github.com/qojulia/SecondQuantizedAlgebra.jl/pull/167)). |
_addto_key!collects like terms and deletes a key when its combined coefficient is zero. It used the structural_iszero_cnumplus an exact-negation fast path (added in #162 forγ/D + (-γ)/D). Both miss cancellations whose two halves are equal but differently represented rationals, e.g.λ/2vs(1//2)λ. Such a term is mathematically zero but survives in theQAdd.Concretely, for a quadratic (squeezing) Hamiltonian
H = Δ·a†a + (λ/2)(a†² + a²):iszeroon that coefficient returnstrue(Symbolics expands), andaveragedrops it, but the operator-levelQAddretained it. Downstream consumers (e.g. QuantumCumulants' cumulant/average_and_truncatepath) then had to special-case the phantom terms, and an exact (no-truncation) mean-field expansion of a quadratic Hamiltonian failed to close because completion chased these never-vanishing higher-order moments.Fix
Add
_iszero_cnum_expand(an expanding zero test) and use it as a final fallback in_addto_key!, gated behind the existing_is_symbolic_cnumguard:After the fix,
commutator(im*H, a*a)returns only the 0th/2nd-order terms, and(λ/2)·aa - (1//2)λ·aacollapses to an empty (iszero)QAdd.