fix(UniTensor): rebuild block metadata instead of mutating shared blocks in-place#998
Conversation
…cks in-place (#724) permute_/reshape_/contiguous_ on a UniTensor whose blocks are shared with another UniTensor (e.g. via relabel(), which is documented to share block storage) corrupted the other holder's shape/metadata, since the in-place Tensor::permute_()/reshape_()/contiguous_() mutate the block's Tensor_impl metadata (shape/mapper/invmapper) directly, and that Tensor_impl can be aliased by several Tensor handles. Fix per the consensus in the issue thread (manuschneider's proposal, endorsed by ianmccul): build new per-block Tensor metadata via the non-mutating Tensor::permute()/reshape()/contiguous() and assign the result back into the block slot (_block / _blocks[i]), instead of calling the in-place Tensor::permute_()/reshape_()/contiguous_(). Storage stays shared (no data copy); only the block's own metadata is rebuilt, so other UniTensors sharing that block keep their original metadata intact. Converted sites: - DenseUniTensor::permute_ (both cytnx_int64- and label-mapper overloads) - DenseUniTensor::reshape_ (non-diag branch) - DenseUniTensor::combineBonds (in-place reshape of _block) - DenseUniTensor::contract (tmpL reshape, which may alias this->_block when already contiguous; the post-Kron "reshape back" is now unneeded since tmpL is rebound rather than mutated) - DenseUniTensor::contiguous_ (include/UniTensor.hpp) - BlockUniTensor::permute_ - BlockUniTensor::combineBonds (in-place reshape of _blocks[b]) - BlockUniTensor::contiguous_ (include/UniTensor.hpp) - BlockFermionicUniTensor::permute_ and permute_nosignflip_ - BlockFermionicUniTensor::combineBonds (in-place reshape of _blocks[b]) - BlockFermionicUniTensor::contiguous_ (include/UniTensor.hpp) Left unconverted (documented in-code as a known follow-up, not fixed here): the block permute_/reshape_ pairs inside BlockUniTensor::contract/BlockFermionicUniTensor::contract that temporarily mutate this->_blocks[a]/Rtn->_blocks[b] in place and restore them afterward (three near-duplicate branches: the UNI_MKL integer-dtype Matmul fallback, the UNI_MKL Gemm_Batch path, and the non-MKL Matmul fallback). Converting these requires threading local Tensor variables through the Gemm_Batch batching loop and dtype-casting/cleanup logic in a hot path; risking a rushed rewrite there was judged worse than leaving it flagged for a follow-up. Tests (gtest, TDD red/green): two UniTensors sharing blocks via relabel(), permute_/reshape_ one and assert the other's shape/labels/data are unchanged and still readable (DenseUniTensor: permute_ and reshape_; BlockUniTensor: permute_; BlockFermionicUniTensor: permute_ incl. the sign-flip state), plus control tests confirming non-shared (cloned) UniTensors are unaffected as before. Full C++ suite: 1088 ran / 1073 passed / 11 skipped / 4 known-failing (pre-existing linalg_Test.BkUt_*Svd_truncate_return_err* failures, unrelated to this change). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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 addresses issue #724 where in-place operations (such as contiguous_(), permute_(), and reshape_()) on a UniTensor sharing underlying block Tensors with another instance (e.g., via relabel()) would corrupt the shared block's metadata. The fix replaces these in-place mutations with non-mutating equivalents and rebinds the block variables. Additionally, comprehensive regression and control tests have been added across BlockFermionicUniTensor, BlockUniTensor, and DenseUniTensor to verify the fix. There are no review comments to address, and I have no further feedback to provide.
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #998 +/- ##
==========================================
+ Coverage 30.88% 30.92% +0.04%
==========================================
Files 229 229
Lines 34758 34761 +3
Branches 14409 14401 -8
==========================================
+ Hits 10734 10751 +17
+ Misses 16720 16699 -21
- Partials 7304 7311 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Code ReviewCore of #724: UniTensors can share block Correctness — verified (read all 13 sites)
Tests — strong and CI-verifiedSix gtests across Dense/Block/BlockFermionic: shared-block Nits
VerdictClean, correct, well-tested foundation fix — the rebind pattern is applied consistently and thoughtfully (including the two sites correctly left alone), the contract() deferral is honest, and it's fully CI-green including BuildAndTest. Approve. Pairs with #1005, which completes contract(). Posted by Claude Code on behalf of @pcchen |
pcchen
left a comment
pcchen
left a comment
There was a problem hiding this comment.
Approving. Correct and well-tested core #724 fix: the non-mutating rebind pattern (_blocks[i] = _blocks[i].permute(...)) is applied consistently across all 13 sites — storage stays shared, metadata detaches — with good judgment on the two sites correctly left in-place (the is_diag/Diag() branch and the terminal Kron scratch). The contract() paths are honestly deferred to #1005 with in-code KNOWN ISSUE breadcrumbs. Six regression gtests (shared + control, cross-holder intactness incl. the fermionic signflip, OOB-guarded reads) are red-proved and pass in CI on all platforms (BuildAndTest green). Only codecov/patch is red (advisory — the deferred KNOWN ISSUE lines).
Posted by Claude Code on behalf of @pcchen
2 participants
Problem
Fixes the core of #724. UniTensors can share block Tensors (e.g.
relabel()copies the block handles), but the in-placepermute_/reshape_/contiguous_/combineBondsimplementations mutated per-block metadata directly — corrupting every other holder of the same block (wrong shapes, out-of-bounds reads).Fix
Per the approach agreed in the issue thread (manuschneider's proposal, endorsed by ianmccul): 13 sites across
DenseUniTensor/BlockUniTensor/BlockFermionicUniTensornow rebuild block metadata via the non-mutatingTensor::permute()/reshape()/contiguous()and rebind (_blocks[i] = _blocks[i].permute(...)) — storage stays shared, metadata detaches. The one deliberate remainder:Block*UniTensor::contract's transient mutate-restore of blocks (three hot-path branches) is marked with in-codeKNOWN ISSUE (#724)blocks and breadcrumbs and is being fixed separately (in-flight follow-up session).Testing
Six new gtests: shared-block
permute_(Dense, Block, BlockFermionic — the latter also pins the signflip vector), shared-blockreshape_(Dense), and two non-shared control tests. Red-proved by reverting fix sites. Full suite: 1073 passed / 11 skipped / only the 4 pre-existing failures addressed by #977.🤖 Generated with Claude Code