Add C++ test coverage for tn_algo and fix the bugs it uncovers (#924) - #927
Closed
IvanaGyro wants to merge 11 commits into
Closed
Add C++ test coverage for tn_algo and fix the bugs it uncovers (#924)#927IvanaGyro wants to merge 11 commits into
IvanaGyro wants to merge 11 commits into
Conversation
RegularMPS::norm() built the <psi|psi> contraction with a hard-coded
label scheme that only closed the trivial dim-1 boundary bonds: the first
site was contracted with its conjugate over the physical and right virtual
legs, and every subsequent site relabelled the running environment to "2"
and contracted it against the new site's left virtual leg. Those two legs
share the label "2" but have different dimensions whenever the virtual bond
is larger than one, so Tensordot aborts with a dimension-mismatch error.
The routine therefore only worked for a product state (chi == 1).
Rebuild the left environment transfer matrix explicitly instead: seed L as
the 1x1 identity on the trivial left boundary, then for each rank-3 site
[left, phys, right] contract the ket left leg onto L's "a", the bra left leg
onto "b", and the two physical legs onto each other, leaving the next
virtual bond open as the new {a, b}. The final 1x1 environment's trace is
<psi|psi>. This contracts correctly for any virtual bond dimension and is
independent of the site tensors' stored labels.
Co-authored-by: Claude <noreply@anthropic.com>
Constructing a regular MPS runs Into_Lortho(), which left-orthogonalises the chain with a sweep of linalg::Svd calls. linalg::Svd always names the new singular-value bond with the fixed labels "_aux_L"/"_aux_R". After the S*vT factor is contracted into the next site, that site's virtual leg keeps the generic "_aux_L" label, which then collides with the "_aux_L" the next Svd reintroduces for its own bond; set_labels rejects the duplicate and the construction aborts (issue #920). RegularMPS was effectively unusable for the regular case as a result. Restore the canonical per-site labels {2k, 2k+1, 2k+2} (the labels Init assigns, where neighbours share their virtual-bond label) after every Svd in Into_Lortho, S_mvright and S_mvleft. Overwriting the bond labels each step stops the generic "_aux_*" labels from surviving into the next decomposition and keeps the chain in a predictable labelling. S_mvright/S_mvleft additionally never changed the orthogonality center: they called the const UniTensor::set_rowrank, which returns a re-rowranked copy and was discarded, instead of the in-place set_rowrank_. The subsequent Svd then split the site at the wrong rowrank. Switch both to set_rowrank_. Co-authored-by: Claude <noreply@anthropic.com>
The DMRG driver was written against older cytnx APIs and crashed before
completing a single sweep. Update it so DMRG runs and converges:
- Network::Launch no longer takes an "optimal order" bool; its argument now
selects the network type, where 1 means Fermion. Every anet.Launch(true)
was therefore selecting an unsupported network type and aborting. Use
Launch() (regular network) throughout.
- The static Network::Contract(...).Launch() helper used in initialize()
leaves the contraction tree's base nodes unallocated and segfaults on
Launch. Replace it with the FromString + PutUniTensors pattern already used
for the same L-A-M-A* environment elsewhere in this file.
- The current RegularNetwork parser splits per-tensor label lines on "," only
and no longer accepts the ";" rowrank marker, so it folded "-4;1" into a
single label and rejected the network. Drop the ";" markers from the tensor
lines (keeping them on TOUT, where they are still honoured).
- set_rowrank is const and returns a copy; the discarded calls left psi and
the boundary tensors at the wrong rowrank for the following Svd. Switch the
in-place uses to set_rowrank_.
- Each Svd_truncate reintroduces the fixed "_aux_L"/"_aux_R" bond labels,
which otherwise survive into the next site's decomposition and collide.
Restore the canonical per-site labels {2k, 2k+1, 2k+2} after every
decomposition (matching the standalone DMRG reference under example/DMRG).
Co-authored-by: Claude <noreply@anthropic.com>
The C++ implementation under src/tn_algo had no test coverage. Add a tn_algo_test suite exercising the public API: - MPS: regular construction (regression for the #920 label-collision crash), metadata and open-boundary shapes, left-orthonormality after Init, normalization, non-uniform physical dimensions, Init_Msector, orthogonality center sweeps via S_mvleft/S_mvright (norm invariance and right- orthonormality), and clone independence. - MPO: append/assign/get_op container behaviour, out-of-bound access, and a check that contracting the matrix-product operator down to a dense matrix reproduces a transverse-field Ising Hamiltonian assembled independently from Pauli operators. - DMRG: convergence to the exact ground-state energy (from dense diagonalization of the same TFIM Hamiltonian) for both sweep() and sweepv2(), at two field strengths. A shared tfim_mpo.hpp helper builds the TFIM MPO tensors, the dense reference Hamiltonian, and its exact ground-state energy. Co-authored-by: Claude <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request resolves issue #920 by ensuring canonical per-site labels are restored after SVD decompositions in both DMRG and RegularMPS, preventing label collisions during sweeps. It also refactors RegularMPS::norm() for a cleaner, site-by-site accumulation of the left environment, simplifies network contractions in DMRG initialization, and updates set_rowrank calls to set_rowrank_. Additionally, a comprehensive suite of unit tests for MPS, MPO, and DMRG has been added. The review feedback suggests improving GPU/CUDA compatibility in RegularMPS::norm() by matching the device of the environment tensor to the MPS tensors, and resolving potential narrowing conversion warnings in the test helper by using cytnx_uint64 instead of int.
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.
The rewritten norm() seeded the 1x1 identity environment with a default (CPU) tensor, so contracting it against MPS tensors living on a GPU would trigger cross-device errors or implicit host/device copies. Create the seed on the same device as the site tensors, falling back to CPU when the chain is empty. Co-authored-by: Claude <noreply@anthropic.com>
Tensor::at takes a std::vector<cytnx_uint64>, so brace-initializing the index list from int loop variables is a narrowing conversion that some compilers reject. Use cytnx_uint64 for the index variables in the TfimW helper. Co-authored-by: Claude <noreply@anthropic.com>
IvanaGyro
marked this pull request as draft
June 22, 2026 02:37
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #927 +/- ##
==========================================
+ Coverage 29.49% 30.65% +1.16%
==========================================
Files 241 241
Lines 35524 35517 -7
Branches 14780 14775 -5
==========================================
+ Hits 10477 10888 +411
+ Misses 17791 17028 -763
- Partials 7256 7601 +345
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 30 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Collaborator
|
If there is no C++ user prepared to rewrite and maintain the |
Address maintainer review on the tn_algo bug-fix changes:
- Underscore-prefix the labels cytnx assigns to MPS site tensors. Init and
Init_Msector now label every site (including site 0) {"_<2k>", "_<2k+1>",
"_<2k+2>"} so the internal bond/physical labels cannot collide with labels a
user might place on the exposed data() tensors.
- Replace the two identical per-file canonical-label helpers with a single
shared CanonicalSiteLabels() in tn_algo/MPS.hpp, used by RegularMPS
(Into_Lortho, S_mvright, S_mvleft) and DMRG. The name follows the Google
style for free functions and takes the index by value.
- In RegularMPS::norm, use explicit UniTensor types instead of auto and
annotate the UniTensor(Tensor, /*is_diag=*/false, /*rowrank=*/1) arguments.
- In DMRG, drop a leftover translation comment, construct the environment
Network with an explicit type instead of auto, and read the local Lanczos
input back into an explicit Tensor instead of auto.
Co-authored-by: Claude <noreply@anthropic.com>
- Move the per-file test fixtures into anonymous namespaces instead of named ones, and drop the unused `using namespace testing;`. - Replace abbreviated identifiers (N, J, h, chi, Dw, ...) with descriptive names (num_sites, coupling, field, bond_dim, ...) across the tests and the shared TFIM helper, and rename the helper entry points accordingly (MakeMpoTensor, DenseHamiltonian). Co-authored-by: Claude <noreply@anthropic.com>
Google C++ style guide disallows using-directives. Drop the file-scope using namespace cytnx; and using namespace cytnx::tn_algo; in MPS_test.cpp, MPO_test.cpp, DMRG_test.cpp, and the function-local using namespace cytnx; in tfim_mpo.hpp, qualifying every Tensor, UniTensor, MPS, MPO, DMRG, and cytnx_uint64/cytnx_int64 use at the call site instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
DMRG::sweep() and DMRG::sweepv2() carry excited-state penalty branches gated on this->ortho_mps being non-empty (the loops over hLRs in initialize(), sweep(), and sweepv2()). The existing ground-state-only tests never populate ortho_mps, so those branches were unexercised. Add TfimTest::SortedEigenvalues/ExactFirstExcitedEnergy and two DMRG tests that compute a ground state, then feed it back in as ortho_mps to drive both sweep paths toward the first excited state, checked against the exact second-lowest eigenvalue of the dense Hamiltonian. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Author
Agree, we can let bot migrate them to python after the PR for stub (#912) is merged. |
DMRG::sweep() and DMRG::sweepv2() guard their per-step progress output behind `if (verbose)`. Every existing tn_algo test passes verbose=false, so those logging branches were never executed. Add DMRG.VerboseLoggingRuns, which drives a small TFIM system with verbose=true through both sweep() and sweepv2(). std::cout is redirected to a local buffer for the duration so the progress logging does not pollute the test output, and the test asserts the buffer is non-empty to confirm the logging path ran. Co-authored-by: Claude <noreply@anthropic.com>
Member
Author
|
C++ algorithm API was removed with #931. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the "add C++ test coverage" part of #924 for
src/tn_algo, and fixes every bug the new tests uncovered (starting with #920). The previously untested C++ implementation ofRegularMPS,MPO/RegularMPO, andDMRGis now exercised and corrected.All 16 new tests pass on both the OpenBLAS (
debug-openblas-cpu) and MKL (debug-mkl-cpu) CPU backends, with AddressSanitizer and leak detection enabled.Tests added (
tests/tn_algo_test/)Init, normalization, non-uniform physical dimensions,Init_Msector, orthogonality-center sweeps viaS_mvleft/S_mvright(norm invariance + right-orthonormality), and clone independence.append/assign/get_opcontainer behaviour, out-of-bound access, and a check that contracting the MPO down to a dense matrix reproduces a transverse-field Ising (TFIM) Hamiltonian assembled independently from Pauli operators.sweep()andsweepv2(), at two field strengths.A shared
tfim_mpo.hpphelper builds the TFIM MPO tensors, the dense reference Hamiltonian, and its exact ground-state energy.Bugs fixed
RegularMPS::normwas broken for any virtual bond dimension > 1 (mismatched-dimension Tensordot); rewrote it as an explicit<ψ|ψ>transfer-matrix contraction that works for any bond dimension.Into_Lortho(and the same class of bug inS_mvright/S_mvleft):linalg::Svd's fixed_aux_L/_aux_Rbond labels survived into the next decomposition and trippedset_labels' duplicate check. Fixed by restoring the canonical per-site labels{2k, 2k+1, 2k+2}after eachSvd. Also fixed a separate no-op where the constset_rowrank(returns a copy) was used instead of in-placeset_rowrank_.Launch(true)selected an unsupported Fermion network; the staticNetwork::Contract().Launch()segfaults (unallocated contraction tree); the;rowrank syntax in network strings is no longer parsed;set_rowrankno-ops; andSvd_truncate's_aux_*labels collided across sites. All fixed; DMRG now converges.Notes / scope
SymMPS.cpp, but there is noSymMPSanywhere in the codebase — the C++MPSonly hasRegularMPSandiMPSimplementations. Coverage targets what actually exists.UniTensor::_Load(SVD-family data-loading tests) andOptimalTreeSolver(SearchTreeTest); these reproduce on the unmodified tree and are outsidetn_algo, so they are left untouched.🤖 Generated with Claude Code