refactor(Bond)!: make Bond immutable; encapsulate BlockFermionicUniTensor signflip#1001
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Bond class to make it immutable, removing all public in-place mutators (such as set_type, redirect_, clear_type, combineBond_, and group_duplicates_) from both C++ and Python APIs. Methods that modify a bond now return a new Bond instance. Additionally, UniTensor's bonds() and signflip_() non-const accessors have been removed or restricted, with internal linalg routines accessing fermionic sign-flips via a friend helper _fermionic_signflip_ to prevent desynchronization. Extensive tests have been updated and added to verify these immutability and desynchronization invariants. I have no additional feedback to provide as there are no review comments to assess.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // blocks (see #841). Not part of the public API: _signflip must stay in lockstep with | ||
| // _blocks/_inner_to_outer_idx and the qnums on the contracted bond, so no other code may | ||
| // touch it. | ||
| std::vector<cytnx_bool> &_fermionic_signflip_(BlockFermionicUniTensor &self); |
There was a problem hiding this comment.
It is discouraged to use variable name prefixed with _. BTW, will we set the style rules in AGENT.md or CLAUDE.md for bots?
There was a problem hiding this comment.
Resolved in a59e3fb by removing the gateway altogether: the _fermionic_signflip_ name no longer exists. The replacements (reset_signflip_() / erase_signflip_()) follow the codebase's trailing-underscore in-place-mutator convention with no leading underscore. (On the side question: #1008 proposes a CLAUDE.md/AGENTS.md with style rules for bots.)
Posted by Claude Code on behalf of @pcchen
| // additional information for fermions: | ||
| std::vector<cytnx_bool> | ||
| _signflip; // if true, the sign of the corresponding block needs to be flipped | ||
| friend std::vector<cytnx_bool> &linalg::_fermionic_signflip_(BlockFermionicUniTensor &self); |
There was a problem hiding this comment.
If others may call this function directly, this should be public not private.
There was a problem hiding this comment.
Agreed, and a59e3fb lands on the public side of this: the friend/private gateway is gone, and the two replacement mutators (reset_signflip_(), erase_signflip_()) are public members -- safe to expose because each preserves the _signflip/_blocks lockstep (erase_signflip_ actively throws on size divergence). _signflip itself stays private with no remaining escape hatch.
Posted by Claude Code on behalf of @pcchen
Code ReviewRemoves Bond's public in-place mutators (Bond is now immutable, so user code can't silently break a UniTensor's symmetry invariant), makes Correctness — verified (high confidence)
Findings (low / notes — none blocking correctness)1. The signflip gateway is a transparent mutable-ref alias, not an invariant-enforcing interface. 2. CI gap — the test evidence is local-only. Wheels compile green, but no 3. Breaking change — needs a changelog/release note + version signal. Well-flagged (title VerdictHigh-quality, careful refactor: Bond immutability is cleanly executed, the signflip encapsulation meets the #841 goal, the surfaced aliasing bug is a genuine fix, and the whole linalg migration is behavior-preserving (independently verified, including the truncation lockstep). Findings are notes, not defects. Approve-with-nits, pending a fresh full-test CI run (the one real gate given the correctness-focused nature of the change). Posted by Claude Code on behalf of @pcchen |
Review update — folding in @IvanaGyro's two comments on the signflip gatewayBoth land on 1. Naming: leading/trailing underscores on the gateway (
|
The correct or better design is to only modify
I see the bot said what mean here. For the naming problem, prefix var names with |
…nsor signflip Bond's public interface can no longer mutate a bond in place: a Bond bound into a UniTensor's block structure could previously be desynchronized via set_type/redirect_/combineBond_/clear_type, silently corrupting the derived _blocks/_inner_to_outer_idx (#846). All four are removed; the return-new forms (retype/redirect/combineBond) are the only way to change a bond's type or combine bonds, so sharing the PIMPL across copies is now safe. Non-const qnums()/syms() and the mutable getDegeneracies() overload are removed for the same reason, as is group_duplicates_() (the in-place counterpart of the kept group_duplicates()). UniTensor's non-const bonds() overload is removed too; internal code that legitimately replaces which bonds a tensor holds now goes through _bonds directly (already public on UniTensor_base) instead of mutating a shared Bond's impl. Bond::clone() deliberately remains a deep copy: #846's suggestion to collapse it to an identity copy is left as a follow-up, because the migrated combine paths now rely on clone-then-mutate of a detached impl (via the internal Bond_impl::force_combineBond_) as their safety argument. Internal call sites that relied on in-place bond mutation (Transpose_, combineBonds, group_basis_ in Dense/Block/BlockFermionicUniTensor; output bond retagging in Svd/Gesvd/Qr/Qdr/Rsvd and their truncate variants) now build new Bond values via retype()/redirect()/combineBond() and assign them back, or clone before calling the internal Bond_impl::force_combineBond_. Per #841, BlockFermionicUniTensor::_signflip moves from public to private. The public signflip_() mutable accessor (UniTensor_base virtual, the BlockFermionicUniTensor override, and the UniTensor wrapper) is deleted; the read-only signflip() stays. The SVD/Gesvd/QR/Eig/Eigh/ExpH/ExpM decomposition internals across src/linalg/*.cpp are the actual encapsulation unit that legitimately reads an input's signflip and initializes/truncates a freshly-built output's signflip in lockstep with its _blocks -- broader than #841's Svd_truncate.cpp-only enumeration, since Qr/Eig/Eigh/ExpH/ExpM/Rsvd construct BlockFermionicUniTensor by hand (never call Init()) and read/write _signflip the same way. They're granted access via a single friend function, linalg::_fermionic_signflip_(), rather than friending each internal helper (several are static/anonymous-namespace and overloaded, which would mean duplicating fragile forward declarations). pybind/bond_py.cpp drops the bindings for the removed mutators (redirect_ was bound by #986); Python users get the return-new forms only, hence the breaking-change marker on this commit. The python group_duplicates_ binding is also removed: it was a pre-existing misbinding of the OUT-of-place C++ Bond::group_duplicates under an in-place-mutator name (pybind treats the mapper output-reference argument as an input, so it never behaved as documented); the non-underscore group_duplicates binding that returns (Bond, mapper) remains. Tests: Bond_test.cpp pins that redirect()/combineBond()/retype() return new values while the receiver (and anything aliasing its impl) is untouched; BlockUniTensor_test.cpp adds a regression that a Bond obtained from a UniTensor can't desync its block structure; Dense/Block/BlockFermionic UniTensor tests add an aliasing regression pinning that a user-held bond copy is not rewritten by combineBond(force=true) (guards the .clone() that detaches the impl before the internal in-place combine); linalg_test.cpp adds a signflip/Nblocks lockstep check on BlockFermionicUniTensor post-truncation. Existing tests that called the removed mutators are rewritten to the return-new forms (including tests/test_apply_pybind.py, which sits outside the pytests/ CI path); pytests/binding_inplace_test.py replaces the in-place redirect_ identity test with a return-new test plus an AttributeError check for the five removed python methods. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mirror the CPU-side tests/Bond_test.cpp fixes: replace the removed Bond::set_type() mutator with retype() in gpu_SimpleBondOperation and gpu_CombineBondNoSymmBraKet, asserting the original bonds stay untouched. tests/gpu/test.cpp:178 still calls set_type, but that file is not referenced by any CMake target (it also uses the long-removed CyTensor class), so it is dead code and left as-is. Verified by clang++ -fsyntax-only against the branch headers (no CUDA toolchain available for a full GPU build): the old file fails with exactly the three set_type errors, the new file compiles clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tests/test_apply_pybind.py compared get_blocks_() pairwise in storage order, but block storage order is not canonical: the tensor produced by BFUT3.permute([3,1,4,2,0]).contiguous().apply() enumerates the same sectors in a different _inner_to_outer_idx order than the manually constructed BFUT3PERM, so the naive zip compared mismatched sectors and the two signflip tests failed. Match blocks by get_qindices() instead, mirroring AreEqUniTensor in tests/test_tools.cpp, and compare (rather than compensate) per-block signflip flags so a tensor with pending flips stays unequal to its applied form and the apply()/apply_() tests cannot pass vacuously. Also move the file to pytests/apply_test.py: CI only runs `pytest pytests/`, so under tests/ it was never executed and had rotted unnoticed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d4c07dc to
18e1218
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1001 +/- ##
==========================================
+ Coverage 57.76% 57.98% +0.21%
==========================================
Files 229 229
Lines 33479 33459 -20
Branches 71 71
==========================================
+ Hits 19340 19401 +61
+ Misses 14117 14036 -81
Partials 22 22
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
…ng mutators Remove the friended linalg::_fermionic_signflip_() accessor that handed out a bare vector<cytnx_bool>& (review feedback: narrowing the access path is not enough if the door it opens is still wide open). Replace it with two members on BlockFermionicUniTensor that cannot desynchronize the #841 _signflip/_blocks lockstep: - reset_signflip_(): sizes the ledger to _blocks.size() right after the decomposition internals (re)build _blocks (13 sites). - erase_signflip_(positions): erases ledger entries with the same index list that erased the blocks during truncation, then throws if the ledger and _blocks disagree in size (18 sites) -- the invariant is now enforced at the hazard point instead of merely hidden. Read-only uses (8 sites) switch to the public signflip() const accessor. No friend declarations remain and _signflip is private with no escape hatch; the reserved-looking gateway name disappears with the gateway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Gateway redesign pushed —
|
Resolves the 4 conflicts from 98 commits of master divergence: - pytests/apply_test.py: take master's canonical `_are_nearly_eq` (the #985 sector-matching helper, functionally identical to this branch's copy), and drop this branch's duplicate. Adapt `_make_bfut3` to the immutable Bond API: `.redirect_()` -> `.redirect()`, since #1001 removes the in-place `redirect_` binding (Python bound only `redirect` on this branch). - src/{DenseUniTensor,BlockUniTensor,BlockFermionicUniTensor}.cpp Transpose_(): keep master's cleaner reverse-index computation (std::ranges iota/reverse), but replace master's in-place `bond.redirect_()` with the immutable `bond = bond.redirect()` this branch requires. Numerically identical (idxnum+1 == rank == _bonds.size()). Verified: the only public Bond mutators #1001 removes are `redirect_` and `combineBonds_`; neither has any remaining live caller after these fixes. openblas-cpu build is clean; full pytests 139 passed / 1 skipped; Transpose_ on tagged/block tensors behaves correctly (bonds redirected, order reversed, original left intact). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Merged
Verified the only public Note for whoever lands this: the apply-test move + helper rewrite are now on master via #985, so this PR no longer carries them — the collision flagged earlier is resolved. |
IvanaGyro
left a comment
IvanaGyro
left a comment
There was a problem hiding this comment.
Better than the original. Other encapsulation issue can be fixed in the future.
| std::vector<cytnx_bool> | ||
| _signflip; // if true, the sign of the corresponding block needs to be flipped | ||
|
|
||
| private: |
There was a problem hiding this comment.
private section should at the end of the class by the style guide.
| def test_bond_removed_mutators_raise_attribute_error(): | ||
| """#846: Bond's in-place mutators (set_type, redirect_, combineBond_, | ||
| clear_type) are removed from the public interface; only the return-new | ||
| forms (retype, redirect, combineBond) remain. group_duplicates_ is also | ||
| gone: it was a misbinding of the out-of-place C++ group_duplicates under | ||
| an in-place-mutator name (the non-underscore group_duplicates stays).""" | ||
| b = cytnx.Bond(2, cytnx.BD_KET) | ||
| for name in ("set_type", "redirect_", "combineBond_", "clear_type", "group_duplicates_"): | ||
| assert not hasattr(b, name), f"Bond.{name} should have been removed" |
There was a problem hiding this comment.
This does not help for the future code.
| Cy_U._impl->_bonds[i] = Cy_U._impl->_bonds[i].retype(Tin.bonds()[i].type()); | ||
| } | ||
| Cy_U.bonds().back().set_type(cytnx::BD_BRA); | ||
| Cy_U._impl->_bonds.back() = Cy_U._impl->_bonds.back().retype(cytnx::BD_BRA); |
There was a problem hiding this comment.
This is still dangerous operation. Changing the bond direction on a block tensor may break symmetries.
There was a problem hiding this comment.
Need to address that block tensors are not fully encapsulated on bond directions.
| vec_erase_(S.get_blocks_(), to_be_removed); | ||
| vec_erase_(S.bonds()[0].qnums(), to_be_removed); | ||
| vec_erase_(S._impl->_bonds[0]._impl->_qnums, to_be_removed); | ||
| if (Tin.uten_type() == UTenType.BlockFermionic) { | ||
| vec_erase_(S.signflip_(), to_be_removed); | ||
| static_cast<BlockFermionicUniTensor *>(S._impl.get())->erase_signflip_(to_be_removed); |
There was a problem hiding this comment.
Allowing to remove blocks in public API of block tensors is also suspicious. Symmetry consistency is not checked by internal functions. It is easy to get wrong for developers. The better workaround may be creating a new one and shadow copy the blocks that need to be preserve. Need to check the shape of block internally while copying.
| void BlockFermionicUniTensor::erase_signflip_(const std::vector<cytnx_uint64> &positions) { | ||
| vec_erase_(this->_signflip, positions); | ||
| cytnx_error_msg(this->_signflip.size() != this->_blocks.size(), | ||
| "[ERROR][BlockFermionicUniTensor][erase_signflip_] after erasing, _signflip " | ||
| "(len %d) is out of lockstep with _blocks (len %d); erase the blocks with the " | ||
| "same index list before erasing the signflips.%s", | ||
| (int)this->_signflip.size(), (int)this->_blocks.size(), "\n"); | ||
| } | ||
|
|
There was a problem hiding this comment.
The better API is remove_blocks, which will also remove signflips. Although, it is still dangerous....
| ====================*/ | ||
| TEST_F(linalg_Test, BkFUt_TruncateKeepsSignflipInLockstepWithNblocks) { | ||
| const double tol = 1e-8; | ||
| UniTensor Msf = permute_with_signflips(make_rank4_herm_dt(false)); | ||
|
|
||
| // keepdim=1 forces the truncation path to actually drop blocks/qnums (not just a no-op pass | ||
| // through), which is when U/S/vT's _signflip must be re-erased in lockstep with _blocks. | ||
| for (auto trunc : {linalg::Svd_truncate(Msf, 1, 0., true, 0), | ||
| linalg::Gesvd_truncate(Msf, 1, 0., true, true, 0)}) { | ||
| ASSERT_EQ(trunc.size(), 3u); // S, U, vT (return_err=0) | ||
| const UniTensor &S = trunc[0]; | ||
| const UniTensor &U = trunc[1]; | ||
| const UniTensor &vT = trunc[2]; | ||
|
|
||
| EXPECT_EQ(S.signflip().size(), S.Nblocks()); | ||
| EXPECT_EQ(U.signflip().size(), U.Nblocks()); | ||
| EXPECT_EQ(vT.signflip().size(), vT.Nblocks()); |
There was a problem hiding this comment.
If there is remove_blocks function in BlockUniTensor, this should be tested on BlockUniTensor instead.
Brings in #1039, #1047 (39 commits). Clean textual merge, but #1026's new test `UniTensor_baseTest.rank_detects_label_bond_mismatch` desyncs bonds/labels via `bonds().pop_back()` -- which no longer compiles now that this PR makes the mutable `bonds()` accessor const (immutable Bond, #846). That desync is unreachable through the public API by design, so the test now pokes the internal `_bonds` vector (const_cast) to still exercise rank()'s defensive size-mismatch guard. Verified: openblas-cpu + RUN_TESTS build clean (CI's editable-install compile gate reproduced locally); UniTensor_base/BlockFermionic/Bond gtests 38 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ss end Review comment (@IvanaGyro): the single `private:` section (the `_signflip` member) was sandwiched between two `public:` blocks. Google style keeps one private section at the end of the class -- move `_signflip` there. Pure reordering; `_signflip` stays private and mutable only via the reset_signflip_()/erase_signflip_() gateway (#841 lockstep unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Brought the branch up to date with master (#1039, #1047) and addressed the review comments: CI failure fixed (was @IvanaGyro "private section should be at the end of the class" — fixed in 764337b: moved Deferred encapsulation comments (Gesvd bond-direction change; |
Brings in the latest master (98de76f): #1001 (Bond made immutable + BlockFermionicUniTensor signflip encapsulated via reset_signflip_/ erase_signflip_), #1047 (Exp/Expf consolidated into one dtype-preserving Exp; Expf/Expf_ deprecated), #1049 (UniTensor//UniTensor floor-division guarded), and the build-test-workflow / cross-revision-benchmark skills. Conflict resolution: - tests/DenseUniTensor_test.cpp: both sides added a test after combineBond_is_in_place -- this PR's combineBond_is_out_of_place and master's #846 aliasing regression (CombineBondForceDoesNotRewriteUserHeldBond). Kept both. Adaptations required by the merge (master's incoming tests were written against master's in-place combineBond(); this PR redefined bare combineBond() as out-of-place per #421/#422): - tests/DenseUniTensor_test.cpp + tests/BlockUniTensor_test.cpp: the #846 CombineBondForceDoesNotRewriteUserHeldBond tests assert the receiver itself mutates (ut.bonds()[0].dim() == combined), so they now call the in-place combineBond_() spelling instead of bare combineBond() (which now returns a new tensor and leaves the receiver unchanged). The #846 aliasing invariant is unchanged -- the in-place force path still clones the bond entry before force_combineBond_. - pytests/apply_test.py: master's newly-moved file called the deprecated .Norm().item(); migrated to .norm() (a native float in Python) to keep the suite clean under -W error::DeprecationWarning, matching this PR's migration of every other in-tree Norm() caller. Auto-merged cleanly (verified as a proper union, no dropped hunks): the PR's norm()->Scalar + underscore renames (set_name_/set_label_/tag_) coexist with master's Bond-immutable retype()/signflip refactor across the linalg decomposition files (Svd/Gesvd/Qr/Qdr/Rsvd/*_truncate); the floordiv guard (UniTensor//UniTensor raises, scalar // kept) sits with the amended #934 elementwise policy in unitensor_py.cpp; Exp consolidation + norm/Norm bindings coexist in linalg.hpp/linalg_py.cpp. Verification (CI-matching openblas-cpu, non-ASan): - C++ test_main: 1248 tests ran, 1237 passed / 11 skipped / 0 failed. - pybind module compiles clean; full pytests/ suite 185 passed / 1 skipped. - merge-adapted test files pass under -W error::DeprecationWarning. - clang-format v14 clean. Note (pre-existing, out of scope; flagged for a follow-up): under an ASan build, BlockUniTensor::combineBonds (src/BlockUniTensor.cpp:1958,1990) trips memcpy-param-overlap -- it memcpy()s an overlapping intra-vector shift where memmove() is required. This is latent UB unchanged by this merge (identical on master); the forward copy yields correct results, so it is benign on the non-ASan CI build and only ASan flags it. master's #846 test (combining two bonds of a 4-bond tensor, leaving a trailing bond) is the first case to surface it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 participants
Problem
Fixes #846 and #841 per the consensus reached in both threads (full public immutability; "correctness guarded by design, not only by tests"). Bond's in-place mutators let user code silently break the block-tensor symmetry invariant of any UniTensor holding that bond, and the public mutable
signflip_()accessor could desynchronizeBlockFermionicUniTensor's_signflip/_blockslockstep.What changed
set_type,redirect_,combineBond_,clear_type,group_duplicates_,operator*=, non-constqnums()/syms(), and the mutablegetDegeneracies()are removed. Return-new forms (retype,redirect,combineBond,group_duplicates) remain and are pinned by new tests.UniTensor::bonds()is const-only; internals migrated._signflipis private (Remove public mutable accessor UniTensor::signflip_(); enforce _signflip invariants inside BlockFermionicUniTensor #841). The issue's enumeration turned out incomplete — the direct-access pattern existed across 8 linalg files, and the per-helper friend design was infeasible (file-static helpers can't be friended) — so a single documented gatewaylinalg::_fermionic_signflip_()is friended once, with the invariant story in its header comment.Bondsurvives aforce=truecombine untouched.AttributeError(their replacements are same-named siblings —redirect,retype,combineBond); the misleadinggroup_duplicates_binding (which never behaved as documented) is also removed.BREAKING CHANGE
C++ and Python code calling the removed Bond mutators must switch to the return-new forms. Release notes should list:
set_type→retype,redirect_→redirect,combineBond_→combineBond(out-of-place),clear_type(construct anew),group_duplicates_→group_duplicates.Testing
C++: 1071 passed / 11 skipped / only the 4 pre-existing failures addressed by #977 (8 new tests incl. a truncation test pinning
signflip().size()==Nblocks()through real Svd/Gesvd truncation, and the aliasing regression pair). pytest: 47 passed. The two known pre-existing failures intests/test_apply_pybind.pyare fixed here (d4c07dc): the_are_nearly_eqhelper comparedget_blocks_()pairwise in storage order, which is not canonical, so it zipped mismatched sectors; it now matches blocks by quantum-number sector viaget_qindices(), mirroring the C++AreEqUniTensor. Unlike the C++ helper it compares per-blocksignflip()flags instead of compensating them — compensation would let a no-opapply_()still compare equal, making the in-place test vacuous. The file also moved topytests/apply_test.pyso CI'spytest pytests/actually runs it (it was silently skipped undertests/).Stacking
Base is
fix/pybind-inplace-return-self(PR #986) — this branch removes pybind mutator bindings that #986 rewrote. Merge #986 first, then retarget to master.🤖 Generated with Claude Code