Skip to content

fix(tests): match blocks by qindices in apply() pybind test helper#985

Merged
yingjerkao merged 3 commits into
masterfrom
claude/upbeat-kepler-72c026
Jul 13, 2026
Merged

fix(tests): match blocks by qindices in apply() pybind test helper#985
yingjerkao merged 3 commits into
masterfrom
claude/upbeat-kepler-72c026

Conversation

@yingjerkao

Copy link
Copy Markdown
Collaborator

Summary

Fixes the two pre-existing failures in tests/test_apply_pybind.py:

  • TestApplyBlockFermionicUniTensor::test_apply_applies_signflips
  • TestApplyBlockFermionicUniTensor::test_apply_inplace_applies_signflips

The backend is correct — the test helper was the bug. T.apply() output was verified element-wise against the hand-built BFUT3PERM reference over the full tensor shape: 0 mismatches, including the expected sign-flipped entries (−5, −6, −7, −8), matching the C++ LinAlgElementwise expectation in tests/BlockFermionicUniTensor_test.cpp.

Root cause

_are_nearly_eq zipped get_blocks_() by list position, but block order is not canonical: permute().contiguous() keeps the source tensor's block enumeration order, while a freshly constructed reference tensor enumerates blocks in canonical qnum order. Positional pairing therefore compared unrelated blocks. The C++ helper AreNearlyEqUniTensor (tests/test_tools.cpp) explicitly matches blocks by quantum numbers ("they might be not in the same order!"), which is why the C++ test passes.

Changes

  • Rewrite _are_nearly_eq to match blocks by get_qindices() and require signflip() flags to agree before comparing raw block data. Deliberately no sign-folding, so the tests stay sensitive to whether apply() actually baked the pending signs in (verified: unapplied T vs reference → False, applied → True).
  • Use the public Bond.redirect_() instead of the c_redirect_ shadow binding (equivalent on master via the Bond_conti.py glue wrapper; avoids a conflict with the in-progress shadow-API removal branch).

Test plan

  • tests/test_apply_pybind.py: 9 passed (was 7 passed / 2 failed)
  • Full pytests/ suite: 28 passed (matches baseline)
  • Test-only change; no C++ touched

🤖 Generated with Claude Code

The two BlockFermionicUniTensor apply()/apply_() signflip tests failed
because _are_nearly_eq compared get_blocks_() by list position, but
block order is not canonical: permute().contiguous() keeps the source
tensor's block enumeration order, while a freshly constructed reference
enumerates blocks in canonical qnum order. The backend apply() output is
element-wise identical to the C++ LinAlgElementwise expectation; only
the comparison was wrong.

Match blocks by get_qindices() and require signflip() flags to agree
before comparing raw block data, mirroring the C++ AreNearlyEqUniTensor
(tests/test_tools.cpp) while staying sensitive to whether apply()
actually folded the pending signs in. Also use the public Bond
redirect_() instead of the c_redirect_ shadow binding.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the _are_nearly_eq helper function in tests/test_apply_pybind.py to match block UniTensors by their quantum-number indices instead of their positions, and to verify that pending signflips agree. Additionally, it replaces the call to c_redirect_() with redirect_() in _make_bfut3(). There are no review comments, so no further feedback is provided.

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.

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.73%. Comparing base (33f5c23) to head (52091dc).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #985      +/-   ##
==========================================
+ Coverage   57.71%   57.73%   +0.02%     
==========================================
  Files         229      229              
  Lines       33468    33468              
  Branches       71       71              
==========================================
+ Hits        19315    19322       +7     
+ Misses      14132    14124       -8     
- Partials       21       22       +1     
Flag Coverage Δ
cpp 57.66% <ø> (+0.01%) ⬆️
python 63.61% <ø> (+0.26%) ⬆️

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

Components Coverage Δ
C++ backend 55.48% <ø> (+0.01%) ⬆️
Python bindings 72.05% <ø> (+0.06%) ⬆️
Python package 63.61% <ø> (+0.26%) ⬆️
see 4 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 33f5c23...52091dc. 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

pcchen commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Rewrites _are_nearly_eq in tests/test_apply_pybind.py to match blocks by quantum-number sector (get_qindices) instead of list position, requiring signflip() agreement before comparing block data; plus c_redirect_redirect_.

Correct as far as it goes

  • The _are_nearly_eq sector-matching + signflip-agreement logic is right — it matches the C++ AreNearlyEqUniTensor (blocks are not in canonical order between permute().contiguous() and a freshly-built reference), and deliberately doesn't sign-fold, so it stays sensitive to whether apply() actually baked the pending signs in. ✅
  • redirect_() is correct on current master (post-refactor(pybind): bind in-place methods directly, drop the c__* shadow API #986, which binds it directly). ✅

🟠 Finding 1 — the fix isn't actually enforced by CI

CI runs pytest pytests/ (ci-cmake_tests.yml:140), and this file lives under tests/, which pytest does not discover there. So tests/test_apply_pybind.py has never been run by CI — that's why the "2 failures" only appear in manual runs, and why #985's green checks don't include them. This PR fixes the helper but leaves the file where CI won't run it, so the now-passing tests remain silently skipped. To make it a real fix, also git mv tests/test_apply_pybind.py pytests/apply_test.py so CI enforces it.

🟠 Finding 2 — duplicates #1001

#1001 (still open, CONFLICTING) does the identical _are_nearly_eq rewrite and the pytests/ file move and the redirect_ change, as part of its larger Bond-immutable work. #985 and #1001 modify the same lines, so they'll conflict — whichever merges second must drop this. Given #1001 is currently stuck (changes-requested + conflicts + gateway-narrowing follow-up), landing #985made complete by adding the file move — is a pragmatic way to fix these tests now, after which #1001 drops its copy.

Recommendation

Good, correct fix — but please add the pytests/ relocation (otherwise it's a no-op in CI), and coordinate with #1001 so the duplicate _are_nearly_eq change doesn't collide.

Posted by Claude Code on behalf of @pcchen

yingjerkao and others added 2 commits July 13, 2026 13:43
The apply() pybind tests lived in tests/test_apply_pybind.py, but CI only
runs `pytest pytests/` (ci-cmake_tests.yml), which does not discover files
under tests/. So the helper fix in this PR was never enforced by CI -- the
now-passing tests stayed silently unrun.

Move the file to pytests/apply_test.py (matching the pytests/ *_test.py
naming) so CI discovers and enforces it. Pure relocation: the test body is
unchanged and self-contained (imports only pytest + cytnx). Verified against
current master: pytests/apply_test.py -> 9 passed; full pytests/ -> 135
passed, 1 skipped.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@yingjerkao

Copy link
Copy Markdown
Collaborator Author

Addressed the review findings:

Finding 1 (fix not enforced by CI) — resolved. Moved tests/test_apply_pybind.pypytests/apply_test.py (commit 52091dc, a 100% rename) so CI's pytest pytests/ discovers and runs it. The branch was also brought up to date with master (168 commits behind → current) so the relocation is verified against today's code, not a stale tree:

  • pytests/apply_test.py9 passed (both previously-failing signflip tests included)
  • full pytests/135 passed, 1 skipped — no collection breakage

Finding 2 (duplicates #1001) — acknowledged. #1001 is still OPEN / CONFLICTING / DIRTY, so landing #985 (now made complete by the file move) remains the pragmatic way to fix these tests. Once #985 merges, #1001 should drop its duplicate _are_nearly_eq rewrite + pytests/ move to avoid the collision.

Posted by Claude Code on behalf of @pcchen

@yingjerkao
yingjerkao merged commit 051d2c8 into master Jul 13, 2026
19 checks passed
@yingjerkao
yingjerkao deleted the claude/upbeat-kepler-72c026 branch July 13, 2026 07:05
yingjerkao added a commit that referenced this pull request Jul 13, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants