fix(symmetry): FermionParitySymmetry::check_qnums rejected all qnums by yingjerkao · Pull Request #1012 · Cytnx-dev/Cytnx

yingjerkao · 2026-07-07T13:54:41Z

Summary

FermionParitySymmetry::check_qnums compared each qnum against this->n, but FermionParitySymmetry sets n = -2 — the fPar stype sentinel from SymmetryType, not a modulus. As a result the plural form returned false for every non-empty input, while the singular check_qnum correctly validated against [0, 2).

Impact: no in-tree caller was affected — Bond validation (src/Bond.cpp) uses the singular check_qnum, which is why fPar bonds have always validated fine. The plural form is, however, exposed to users via the Python bindings (Symmetry.check_qnums), where it silently rejected valid parity qnum lists.

Fix

Reimplement check_qnums as a loop over check_qnum so the two forms cannot diverge again.

Tests

New tests/Symmetry_test.cpp (registered in tests/CMakeLists.txt) covering both forms for fPar:

check_qnums({0, 1}) == true, check_qnums({2}) == false, plus {-1} and {0, 1, 2} rejection
singular check_qnum for 0/1 accepted, -1/2 rejected

Verified red-green: the check_qnums({0, 1}) assertion fails before the fix and passes after.

Note for #842

The value-type Symmetry refactor branch (refactor/symmetry-value-type, #842) deliberately preserved this quirk byte-for-byte; it should pick up this fix when rebasing.

🤖 Generated with Claude Code

check_qnums compared each qnum against this->n, but FermionParitySymmetry sets n = -2 (the fPar stype sentinel, not a modulus), so the plural form returned false for every non-empty input while the singular check_qnum correctly validated against [0, 2). No in-tree caller was affected (Bond validation uses the singular form), but the plural form is exposed via the Python bindings. Reimplement check_qnums as a loop over check_qnum so the two cannot diverge again, and add a gtest covering both forms. Note: the value-type Symmetry refactor branch (refactor/symmetry-value-type, #842) deliberately preserved this quirk byte-for-byte and will need to pick up this fix on rebase. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request refactors FermionParitySymmetry::check_qnums in src/Symmetry.cpp to simplify the validation logic by delegating to check_qnum and returning early on failure. It also introduces unit tests for these checks in a new test file tests/Symmetry_test.cpp and registers it in tests/CMakeLists.txt. The reviewer suggested using a range-based for loop in check_qnums to make the code more idiomatic and avoid potential type mismatches.

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.

gemini-code-assist · 2026-07-07T13:55:23Z

    for (cytnx_uint64 i = 0; i < qnums.size(); i++) {
-      out = ((qnums[i] >= 0) && (qnums[i] < this->n));
-      if (out == false) break;
+      if (!this->check_qnum(qnums[i])) return false;
    }


Using a range-based for loop is more idiomatic in modern C++ and avoids potential type mismatch issues between cytnx_uint64 and std::vector::size_type (which is size_t).

Suggested change

for (cytnx_uint64 i = 0; i < qnums.size(); i++) {

out = ((qnums[i] >= 0) && (qnums[i] < this->n));

if (out == false) break;

if (!this->check_qnum(qnums[i])) return false;

}

for (const auto &qnum : qnums) {

if (!this->check_qnum(qnum)) return false;

}

Applied in 816e73d — switched to the range-based for. Behavior is unchanged (still delegates to check_qnum and short-circuits on the first invalid qnum); this just drops the cytnx_uint64 vs size_t index-type mismatch. clang-format (v14) clean.

codecov · 2026-07-07T14:09:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 32.67%. Comparing base (d6dcd16) to head (816e73d).
⚠️ Report is 32 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1012      +/-   ##
==========================================
+ Coverage   31.82%   32.67%   +0.84%     
==========================================
  Files         230      230              
  Lines       33124    33001     -123     
  Branches    13852    13746     -106     
==========================================
+ Hits        10543    10784     +241     
+ Misses      15539    14957     -582     
- Partials     7042     7260     +218

Flag	Coverage Δ
cpp	`32.31% <100.00%> (+0.85%)`	⬆️
python	`61.84% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
C++ backend	`33.53% <100.00%> (+0.98%)`	⬆️
Python bindings	`24.03% <ø> (+0.01%)`	⬆️
Python package	`61.84% <ø> (ø)`

Files with missing lines	Coverage Δ
src/Symmetry.cpp	`34.28% <100.00%> (+2.26%)`	⬆️

... and 55 files with indirect coverage changes

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6dcd16...816e73d. Read the comment docs.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pcchen · 2026-07-08T02:37:49Z

Code Review — LGTM

The bug (confirmed): the plural form tested qnums[i] < this->n, but FermionParitySymmetry carries n = -2 (the fPar stype sentinel, not a modulus) — so the test was always false and check_qnums returned false for every non-empty input. The singular check_qnum was always correct: (qnum >= 0) && (qnum < 2) (verified on master). No in-tree caller was affected (Bond validation uses the singular), but the plural is Python-exposed via Symmetry.check_qnums, where it silently rejected valid parity lists.

The fix (verified): reimplements the plural as a loop over check_qnum — a structural fix, not a patched constant: the two forms cannot diverge again (single source of truth). Empty-input semantics preserved (both old and new return true).

Tests: both forms pinned for fPar — singular 0/1 accept, −1/2 reject; plural {0,1} pass, {2} / {-1} / {0,1,2} reject (the last catching the not-just-first-element case). Red-green verified. CI fully green (16 pass) including BuildAndTest running the new tests.

Coordination note (🟡)

This collides with open #1010 (Symmetry → variant), which (1) also creates tests/Symmetry_test.cpp + the same CMakeLists line (both-added conflict; resolution: merge the two test files, dedupe the CMake line), and (2) deliberately pinned this very bug bit-for-bit as a preserved quirk — after this lands, #1010's quirk pin will fail and must flip to the fixed expectation, with the fix re-expressed inside its rewritten kind-struct check_qnums. The PR body already anticipates this. Recommended order (same logic as #984-before-#1011): land this small green bugfix first; #1010's rebase absorbs it.

No findings on the change itself. LGTM.

Posted by Claude Code on behalf of @pcchen

@gemini-code-assist

…ck_qnums Address @gemini-code-assist review on #1012: a range-based loop is more idiomatic and sidesteps the cytnx_uint64 vs std::vector::size_type (size_t) index-type mismatch. Pure cleanup — behavior is unchanged (still delegates to check_qnum, still short-circuits on the first invalid qnum). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

pcchen

Approving. Confirmed bug (the plural form tested qnums[i] < this->n against the n = -2 fPar sentinel — always false for non-empty input) and a structural fix: the plural now delegates per-element to the always-correct singular check_qnum ([0,2) range), so the two forms cannot diverge again. Empty-input semantics preserved; both forms pinned by tests (including the {0,1,2} not-just-first-element case); CI fully green on the current head (the follow-up commit is a cosmetic range-for conversion, semantically identical).

Coordination note stands: #1010 quirk-pinned this exact behavior and adds the same test file — its rebase should flip the pin and merge the two Symmetry_test.cpp contents.

Posted by Claude Code on behalf of @pcchen

gemini-code-assist Bot reviewed Jul 7, 2026

View reviewed changes

pcchen approved these changes Jul 8, 2026

View reviewed changes

pcchen merged commit 8f69f05 into master Jul 8, 2026
19 checks passed

pcchen deleted the fix/fpar-check-qnums branch July 8, 2026 13:31

pcchen mentioned this pull request Jul 8, 2026

refactor(Symmetry): make Symmetry a plain value type over std::variant #1010

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(symmetry): FermionParitySymmetry::check_qnums rejected all qnums#1012

fix(symmetry): FermionParitySymmetry::check_qnums rejected all qnums#1012
pcchen merged 2 commits into
masterfrom
fix/fpar-check-qnums

yingjerkao commented Jul 7, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Jul 7, 2026

Uh oh!

yingjerkao Jul 8, 2026

Uh oh!

codecov Bot commented Jul 7, 2026 •

edited

Loading

Uh oh!

pcchen commented Jul 8, 2026

Uh oh!

pcchen left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

yingjerkao commented Jul 7, 2026

Summary

Fix

Tests

Note for #842

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

yingjerkao Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pcchen commented Jul 8, 2026

Code Review — LGTM

Coordination note (🟡)

Uh oh!

pcchen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 7, 2026 •

edited

Loading