refactor(Symmetry): make Symmetry a plain value type over std::variant#1010
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 Symmetry class from an intrusive pointer-based implementation (boost::intrusive_ptr) to a plain value type using std::variant and std::visit. This change makes Symmetry objects independent copies upon assignment and copy construction. The PR also adds comprehensive unit tests in tests/Symmetry_test.cpp to ensure byte-compatibility and API correctness. The review feedback suggests improving const-correctness across several member functions in Symmetry (such as check_qnum, check_qnums, combine_rule, combine_rule_, and reverse_rule_) by marking them as const.
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.
| bool check_qnum(const cytnx_int64 &qnum) { | ||
| return std::visit([&](const auto &kind) { return kind.check_qnum(qnum); }, this->_impl); | ||
| } |
There was a problem hiding this comment.
The check_qnum method does not modify the Symmetry object and should be marked const. This ensures const-correctness and allows the method to be called on const Symmetry instances, which is a common use case.
| bool check_qnum(const cytnx_int64 &qnum) { | |
| return std::visit([&](const auto &kind) { return kind.check_qnum(qnum); }, this->_impl); | |
| } | |
| bool check_qnum(const cytnx_int64 &qnum) const { | |
| return std::visit([&](const auto &kind) { return kind.check_qnum(qnum); }, this->_impl); | |
| } |
| bool check_qnums(const std::vector<cytnx_int64> &qnums) { | ||
| return this->_impl->check_qnums(qnums); | ||
| return std::visit([&](const auto &kind) { return kind.check_qnums(qnums); }, this->_impl); | ||
| } |
There was a problem hiding this comment.
The check_qnums method does not modify the Symmetry object and should be marked const to ensure const-correctness and allow usage on const Symmetry instances.
| bool check_qnums(const std::vector<cytnx_int64> &qnums) { | |
| return this->_impl->check_qnums(qnums); | |
| return std::visit([&](const auto &kind) { return kind.check_qnums(qnums); }, this->_impl); | |
| } | |
| bool check_qnums(const std::vector<cytnx_int64> &qnums) const { | |
| return std::visit([&](const auto &kind) { return kind.check_qnums(qnums); }, this->_impl); | |
| } |
| std::vector<cytnx_int64> combine_rule(const std::vector<cytnx_int64> &inL, | ||
| const std::vector<cytnx_int64> &inR) { | ||
| return this->_impl->combine_rule(inL, inR); | ||
| std::vector<cytnx_int64> out; | ||
| this->combine_rule_(out, inL, inR); | ||
| return out; | ||
| } |
There was a problem hiding this comment.
The vector overload of combine_rule does not modify the Symmetry object and should be marked const to match the scalar overload and ensure const-correctness.
| std::vector<cytnx_int64> combine_rule(const std::vector<cytnx_int64> &inL, | |
| const std::vector<cytnx_int64> &inR) { | |
| return this->_impl->combine_rule(inL, inR); | |
| std::vector<cytnx_int64> out; | |
| this->combine_rule_(out, inL, inR); | |
| return out; | |
| } | |
| std::vector<cytnx_int64> combine_rule(const std::vector<cytnx_int64> &inL, | |
| const std::vector<cytnx_int64> &inR) const { | |
| std::vector<cytnx_int64> out; | |
| this->combine_rule_(out, inL, inR); | |
| return out; | |
| } |
| void combine_rule_(std::vector<cytnx_int64> &out, const std::vector<cytnx_int64> &inL, | ||
| const std::vector<cytnx_int64> &inR) { | ||
| this->_impl->combine_rule_(out, inL, inR); | ||
| std::visit([&](const auto &kind) { kind.combine_rule_(out, inL, inR); }, this->_impl); | ||
| } |
There was a problem hiding this comment.
The vector overload of combine_rule_ does not modify the Symmetry object and should be marked const to ensure const-correctness.
| void combine_rule_(std::vector<cytnx_int64> &out, const std::vector<cytnx_int64> &inL, | |
| const std::vector<cytnx_int64> &inR) { | |
| this->_impl->combine_rule_(out, inL, inR); | |
| std::visit([&](const auto &kind) { kind.combine_rule_(out, inL, inR); }, this->_impl); | |
| } | |
| void combine_rule_(std::vector<cytnx_int64> &out, const std::vector<cytnx_int64> &inL, | |
| const std::vector<cytnx_int64> &inR) const { | |
| std::visit([&](const auto &kind) { kind.combine_rule_(out, inL, inR); }, this->_impl); | |
| } |
| void combine_rule_(cytnx_int64 &out, const cytnx_int64 &inL, const cytnx_int64 &inR, | ||
| const bool &is_reverse = false) { | ||
| this->_impl->combine_rule_(out, inL, inR, is_reverse); | ||
| std::visit([&](const auto &kind) { kind.combine_rule_(out, inL, inR, is_reverse); }, | ||
| this->_impl); | ||
| } |
There was a problem hiding this comment.
The scalar overload of combine_rule_ does not modify the Symmetry object and should be marked const to ensure const-correctness.
| void combine_rule_(cytnx_int64 &out, const cytnx_int64 &inL, const cytnx_int64 &inR, | |
| const bool &is_reverse = false) { | |
| this->_impl->combine_rule_(out, inL, inR, is_reverse); | |
| std::visit([&](const auto &kind) { kind.combine_rule_(out, inL, inR, is_reverse); }, | |
| this->_impl); | |
| } | |
| void combine_rule_(cytnx_int64 &out, const cytnx_int64 &inL, const cytnx_int64 &inR, | |
| const bool &is_reverse = false) const { | |
| std::visit([&](const auto &kind) { kind.combine_rule_(out, inL, inR, is_reverse); }, | |
| this->_impl); | |
| } |
| void reverse_rule_(cytnx_int64 &out, const cytnx_int64 &in) { | ||
| this->_impl->reverse_rule_(out, in); | ||
| std::visit([&](const auto &kind) { kind.reverse_rule_(out, in); }, this->_impl); | ||
| } |
There was a problem hiding this comment.
The reverse_rule_ method does not modify the Symmetry object and should be marked const to ensure const-correctness.
| void reverse_rule_(cytnx_int64 &out, const cytnx_int64 &in) { | |
| this->_impl->reverse_rule_(out, in); | |
| std::visit([&](const auto &kind) { kind.reverse_rule_(out, in); }, this->_impl); | |
| } | |
| void reverse_rule_(cytnx_int64 &out, const cytnx_int64 &in) const { | |
| std::visit([&](const auto &kind) { kind.reverse_rule_(out, in); }, this->_impl); | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1010 +/- ##
==========================================
+ Coverage 32.99% 33.15% +0.15%
==========================================
Files 230 230
Lines 33076 33031 -45
Branches 13787 13794 +7
==========================================
+ Hits 10915 10950 +35
+ Misses 14804 14702 -102
- Partials 7357 7379 +22
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Code ReviewConverts Correctness — verified
Findings (low / notes — none blocking)1. A second reference->value semantic change, only partly documented. The PR flags that 2. 3. Stale comment 4. Preserved pre-existing bugs ( VerdictHigh-quality, faithful refactor: the variant model is cleaner and cheaper than the virtual PIMPL, the public API and Save/Load byte format are preserved (structurally verified here; pinned by base-generated fixtures in the suite), and the semantic changes are deliberate and mostly documented. Findings are notes/nits, not defects. Approve-with-nits once Posted by Claude Code on behalf of @pcchen |
Correction + verification follow-up to my review aboveRetracting finding #3 — it was my error. I claimed Byte-compat and API preservation now positively verified (I did a deeper pass on the fixtures):
Net: the two nits that stand are #1 (document the Posted by Claude Code on behalf of @pcchen |
On @gemini-code-assist's 6
|
Status update: #1012 has merged — three rebase items now apply here#1012 (
Standing from earlier review, unchanged: approve-with-nits verdict, Gemini's six Posted by Claude Code on behalf of @pcchen |
Consolidated checklist to land this PREverything below is gathered from the review, the const-correctness thread, and the #1012 merge — nothing else is outstanding. A. Rebase onto current master (currently conflicting) — three specific resolutions
B. Const-correctness (the six suggestions, endorsed)
C. Documentation
D. After rebase
Follow-up (not this PR): retire the Everything else was already verified in review (byte-compat pinned against real old-format fixtures, full API surface preserved, Posted by Claude Code on behalf of @pcchen |
#842) Replace the boost::intrusive_ptr<Symmetry_base> hierarchy (4 virtual subclasses, heap allocation, refcounting) with four small value structs held inline in a std::variant: Symmetry::kind_variant = std::variant<U1Symmetry, ZnSymmetry, FermionParitySymmetry, FermionNumberSymmetry> Every public method dispatches via std::visit; per the design endorsed in issue #842, the visit lambdas are the completeness contract: adding a fifth alternative that does not implement the full rule set (check_qnum, check_qnums, both combine_rule_ overloads, reverse_rule_, get_fermion_parity, is_fermionic, print_info, stype_str) fails to compile. This exhaustiveness guarantee is compile-time only and cannot be pinned by a runtime test. Each kind keeps the legacy `n` field with the value the old implementation stored (U1: 1, Zn: n, fPar: -2, fNum: -1); it backs the preserved `int &n() const` accessor and the Save/Load byte format. The field is declared `mutable` so the legacy accessor (a mutable reference handed out from a const Symmetry, exactly the mutability the shared impl exposed) stays well-defined even for genuinely const objects, with no const_cast. The signature itself is pinned by a static_assert in tests/Symmetry_test.cpp; accessor cleanup is tracked with the #840-era API follow-ups. Public API preserved exactly: factories U1()/Zn(n)/FermionParity()/ FermionNumber(), the (stype, n) constructor and Init, stype()/ stype_str()/n(), check_qnum(s), combine_rule and the combine_rule_ out-param forms (issue #840's redesign is out of scope here), reverse_rule(_), get_fermion_parity, is_fermionic, print_info, operator==/!= (stype + n), Save/Load/_Save/_Load, the SymmetryType constants, all reachable error messages, and the pybind surface (symmetry_py.cpp unchanged). clone() is now a plain copy. The only dropped code is the dead U1Symmetry(int) checking constructor (the wrapper always constructed U1 with n = 1; zero callers). Immutability audit: no in-tree code mutates a Symmetry after construction. Init()/_Load() always rebound the impl pointer (never mutated the shared pointee), nothing assigns through n(), and nothing reaches into _impl except utils/is.cpp. Value copies are therefore observationally identical to the old shared-impl copies, with one deliberate exception: is(a, b) for Symmetry now means "same object" (&L == &R) instead of "shares the impl pointer", so a copy is no longer `is` its source. The only in-tree observer was the doc example example/Symmetry/clone.cpp, updated together with its .out. Byte compatibility is pinned by committed fixtures: tests/test_data_base/common/Symmetry/ holds .cysym files for all four kinds plus a .cybd Bond carrying all four symmetries, written through the public Save API by the pre-refactor implementation at the base commit d6dcd16 (regeneration recipe: CommonDataGen.Symmetry_gen in tests/common_data_generator.cpp). tests/Symmetry_test.cpp loads each fixture, checks stype/n/behavior against freshly constructed objects, and byte-compares fresh Saves against the fixture files (magic 777:u32 + stype:i32 + n:i32; Bond container format). The core 18 tests ran green on the base build first, then unchanged on this refactor -- both directions of the serialization contract hold. A bad-magic Load error-path test and the n() signature static_assert round out the suite. Test tally: C++ 1121 ran / 1106 passed / 11 skipped / 4 known linalg_Test.BkUt_* failures (baseline 1101/1086/11/4 plus 20 new tests); pytest 57 passed, unchanged. Preserved bit-for-bit and pinned by Symmetry.FermionParityCheckQnumsLegacyQuirk: FermionParitySymmetry::check_qnums still compares against n = -2 and so returns false for every non-empty input, while the singular check_qnum uses the correct [0, 2) range. Pre-existing bug, tracked separately; do not fix without updating that test. Refs #842. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2b82c48 to
bfa0e84
Compare
|
Checklist item A implemented — rebased onto master (head
Sanity: no conflict markers, brace balances clean, byte-compat fixtures untouched. Fresh CI is running on Posted by Claude Code on behalf of @pcchen |
Apply the six const-correctness suggestions from review: check_qnum, check_qnums, the vector combine_rule overload, both combine_rule_ out-param overloads, and reverse_rule_ on the Symmetry wrapper are now const. All six dispatch via std::visit with `const auto &kind` onto kind-struct methods that are already const, so visiting a const _impl is well-formed; get_fermion_parity and is_fermionic were already const, making the read-only rule surface uniformly callable on const Symmetry objects. pybind bindings are unaffected (const member pointers / lambdas on non-const self). This is a deliberate, non-breaking addition on top of the PR's signature-preservation goal (const-qualification only relaxes callability). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Checklist item B implemented (commit
Remaining: C (changelog entry for the Posted by Claude Code on behalf of @pcchen |
…-decisions The value-type refactor has two observable reference->value changes; only is() was noted. Add the second alongside it: the legacy `int &n() const` reference now points into this Symmetry's own variant, so writing through it no longer propagates to copies (under the shared intrusive-ptr impl it did). Documented at the accessor and recorded, together with the is() change and the byte-format preservation, as an entry in docs/dev/api-decisions.md (the repo has no CHANGELOG file; api-decisions.md is the release-notes source for API-semantics changes per #1006). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Checklist item C implemented (commit
Checklist status: A ✅ B ✅ C ✅ — only D remains (fresh CI green, running on Posted by Claude Code on behalf of @pcchen |
pcchen
left a comment
pcchen
left a comment
There was a problem hiding this comment.
Approving. The value-type refactor was verified in review (index-aligned variant pinned by static_asserts; Save/Load byte format pinned against real old-format fixtures generated at the base commit, byte-compared both directions; public API preserved signature-for-signature — with the one deliberate, documented exception of const-qualifying the non-mutating rule methods). The full review checklist is complete:
- A — rebased onto master with the #1012 fix ported into the variant
check_qnums(avoiding silent bug re-introduction), the preserved-quirk pin flipped to the fixed expectations, the twoSymmetry_test.cppsuites merged, and the CMakeLists entry deduped. - B — the six const-correctness suggestions applied (+
get_fermion_parity/is_fermionicverified already const); pybind member-pointer bindings unaffected. - C — the
n()copy-independence change documented at the accessor and recorded with theis()change indocs/dev/api-decisions.md; PR description updated (including correcting the now-stale preserved-quirk claim). - D — CI fully green on
15b2d99e(16 pass; the single ubuntu failure was a conda env-setup infra flake, passed on rerun).
Note for the record: the checklist commits (A–C) were implemented by Claude Code on this branch per the review threads; the core refactor is @yingjerkao's.
Posted by Claude Code on behalf of @pcchen
2 participants
Problem
Fixes #842.
Symmetrywas aboost::intrusive_ptr<Symmetry_base>PIMPL over four virtual subclasses — reference semantics, virtual dispatch, andclone()boilerplate for what is a small immutable value.What changed
Symmetrynow holds a by-valuestd::variant<U1Symmetry, ZnSymmetry, FermionParitySymmetry, FermionNumberSymmetry>; every method dispatches viastd::visit. The generic visit lambdas are the completeness contract: adding a fifth kind fails compilation until all rule methods exist.Preserved exactly: the full public API (verified signature-by-signature — factories,
combine_rule(_)including out-param forms,stype()/stype_str(), the legacyint& n() constaccessor now backed by amutablemember, comparison = stype+n, all error messages) and the Save/Load byte format — pinned by binary fixtures generated at the base commit and byte-compared in tests, both directions, including a mixed-symmetry Bond. (One deliberate exception, per review: the non-mutating rule methods —check_qnum,check_qnums, the vectorcombine_rule, bothcombine_rule_overloads,reverse_rule_— are nowconst-qualified. This only relaxes callability; no signature is otherwise changed.)Deliberate notes:
is(Symmetry, Symmetry)now means address identity (copies are no longeristheir source); likewise, writing through the legacyint &n() constreference no longer propagates to copies (each Symmetry owns its value) — the honest semantics for a value type; the sole in-tree observer (a doc example) is updated. Of the two pre-existing bugs found during the work,FermionParitySymmetry::check_qnums(always false for non-empty input) has since been fixed on master by #1012 and this branch ports that fix into the variant implementation; theNEED_GEN_COMMON_DATA=1exit crash remains preserved and tracked separately.Testing
20 new gtests (byte-compat fixtures, API pinning, quirk pin, bad-magic Load rejection, signature
static_assert). Full suite: 1106 passed / 11 skipped / only the 4 pre-existing failures addressed by #977. pytest 57 passed; the pybind surface diff is empty.🤖 Generated with Claude Code