fix(tests): match blocks by qindices in apply() pybind test helper#985
Conversation
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>
There was a problem hiding this comment.
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 Report✅ All modified and coverable lines are covered by tests. 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
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:
|
Code ReviewRewrites Correct as far as it goes
🟠 Finding 1 — the fix isn't actually enforced by CICI runs 🟠 Finding 2 — duplicates #1001#1001 (still open, RecommendationGood, correct fix — but please add the Posted by Claude Code on behalf of @pcchen |
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>
|
Addressed the review findings: Finding 1 (fix not enforced by CI) — resolved. Moved
Finding 2 (duplicates #1001) — acknowledged. #1001 is still Posted by Claude Code on behalf of @pcchen |
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>
Summary
Fixes the two pre-existing failures in
tests/test_apply_pybind.py:TestApplyBlockFermionicUniTensor::test_apply_applies_signflipsTestApplyBlockFermionicUniTensor::test_apply_inplace_applies_signflipsThe backend is correct — the test helper was the bug.
T.apply()output was verified element-wise against the hand-builtBFUT3PERMreference over the full tensor shape: 0 mismatches, including the expected sign-flipped entries (−5, −6, −7, −8), matching the C++LinAlgElementwiseexpectation intests/BlockFermionicUniTensor_test.cpp.Root cause
_are_nearly_eqzippedget_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++ helperAreNearlyEqUniTensor(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
_are_nearly_eqto match blocks byget_qindices()and requiresignflip()flags to agree before comparing raw block data. Deliberately no sign-folding, so the tests stay sensitive to whetherapply()actually baked the pending signs in (verified: unappliedTvs reference →False, applied →True).Bond.redirect_()instead of thec_redirect_shadow binding (equivalent on master via theBond_conti.pyglue 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)pytests/suite: 28 passed (matches baseline)🤖 Generated with Claude Code