Rsvd for symmetric tensors

[manuschneider]

Implementing Rsvd for symmetric tensors
Cleaning up Svd, Gesvd, Rsvd and unifying the implementations

[codecov]

Codecov Report

❌ Patch coverage is 72.08498% with 565 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.07%. Comparing base (fa015d4) to head (55a5868).
⚠️ Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
src/linalg/Svd.cpp	50.14%	168 Missing and 7 partials ⚠️
src/linalg/Rsvd.cpp	64.62%	58 Missing and 69 partials ⚠️
src/linalg/Gesvd_truncate.cpp	60.82%	54 Missing and 51 partials ⚠️
src/linalg/Svd_truncate.cpp	62.40%	50 Missing and 47 partials ⚠️
src/linalg/Gesvd.cpp	90.05%	20 Missing and 15 partials ⚠️
src/linalg/Rsvd_notruncate.cpp	94.02%	6 Missing and 20 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #744      +/-   ##
==========================================
+ Coverage   35.44%   37.07%   +1.62%     
==========================================
  Files         215      214       -1     
  Lines       33071    33573     +502     
  Branches    13170    13531     +361     
==========================================
+ Hits        11723    12446     +723     
+ Misses      19424    19115     -309     
- Partials     1924     2012      +88     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ianmccul]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 519cbf74b1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

To use Codex here, create a Codex account and connect to github.

[pcchen]

Code Review: PR #744 "Rsvd for Symmetric Tensors"

Critical Issues

1. GPU shape mismatch in cuQuantum path (Rsvd_truncate.cpp:70–100)
When samplenum < n_singlu, in is projected to [samplenum × original_cols], but U, S, vT are pre-allocated for the original dimensions before projection. Passing mismatched buffers to cuQuantumGeSvd risks memory corruption or incorrect results.

2. Block/BlockFermionic paths are dead code (Rsvd.cpp:218–224)
The PR is titled "Rsvd for Symmetric Tensors" but the Block/BlockFermionic branches are commented out — any call on a symmetric tensor throws a runtime error. The public header has no documentation warning users it's Dense-only. Either complete the feature or document the restriction clearly.

3. Missing semicolon after cytnx_error_msg (Gesvd_truncate.cpp:361–364)

cytnx_error_msg(true, "[ERROR][_gesvd_truncate_BlockFermionic_UT] not implemented...", "\n")
cytnx_uint64 keep_dim = keepdim;  // ← missing ; above — syntax hazard

Depending on macro expansion, this either causes a compile error or silently absorbs the next statement.

4. Helper functions not in unnamed namespaces
_gesvd_truncate_Block_UT (Gesvd_truncate.cpp:210,553) and _svd_truncate_Block_UTs (Svd_truncate.cpp:178,378) are in the public cytnx::linalg namespace instead of an unnamed namespace — polluting the ABI. The PR description said helpers would be moved to unnamed namespaces, but this wasn't completed.

---

Important Issues

5. No upper-bound check on keepdim (Rsvd.cpp:28)
Only keepdim < 1 is validated. If keepdim > min(rows, cols), the user silently gets a full SVD with no warning. Rsvd_truncate.cpp has an explicit fallback for this — Rsvd.cpp should match.

6. smidx out-of-bounds when min_dim is negative (Gesvd_truncate.cpp:629–638)
When min_blockdim entries exceed keepdim + mindim, min_dim can go negative. The while loop condition keep_dim > min_dim can then allow smidx++ beyond the bounds of Sall. Identical issue in Svd_truncate.cpp:458.

7. Definition order issue with overloaded helpers
In both Gesvd_truncate.cpp and Svd_truncate.cpp, the second dispatcher calls a helper overload defined after the dispatcher in the same file. Compiles fine but is fragile and violates usual style.

---

Minor Issues

8. Docstring typo (linalg.hpp:847): "UniYrndot" → "UniTensor"

9. Dead commented-out duplicate calls in Rsvd.cpp, Gesvd.cpp, Svd.cpp — copy-paste artifacts from refactoring.

10. memcpy type mismatch (Rsvd_truncate.cpp:150–204): oldshape is vector<cytnx_uint64> but memcpy target is cytnx_int64. Technically UB though benign in practice on 64-bit platforms.

---

GPU Bug Fix (Confirmed Correct)

The reported uninitialized Q bug is properly fixed — both the cuQuantum path and the CPU fallback GPU path correctly guard Matmul(Q, U) behind the samplenum < n_singlu condition.

---

Summary

Priority	Count	Key Blocker
Critical	4	GPU shape mismatch, dead feature, missing semicolon, ABI leakage
Important	3	Missing keepdim validation, smidx OOB, definition order
Minor	3	Typo, dead comments, memcpy type mismatch

The two most urgent blockers before merging: GPU shape mismatch in the cuQuantum path (issue #1) and the missing semicolon (issue #3) which may prevent compilation of the BlockFermionic stub.

Review generated with Claude Code

[manuschneider]

Code Review: PR #744 "Rsvd for Symmetric Tensors"

1. Fixed
2. • 4. Either simply wrong or old code, not an issue with the newest version on this branch.
3. keepdim can be larger than the rank without issues. In this case, a full SVD will be done in Rsvd_notruncate, which is fine.
4. In the loop, min_dim can never be negative because of the lines

if (!any_min_blockdim) {
  // make sure that at least one singular value is kept
  min_dim = (min_dim < 1 ? 1 : min_dim);
} else {
  min_dim = (min_dim < 1 ? 0 : min_dim);
}

7. Not the case, see 2. - 4.
8. Fixed.
9. Removed unnecessary code in Rsvd.
10. Fixed.

[pcchen]

Critical Issues Status — Follow-up Review

Posted by Claude Code on behalf of @pcchen

Critical Issue #1: GPU shape mismatch in cuQuantum path

NOT RESOLVED

src/linalg/Rsvd.cpp:78–81 — when projection is applied (samplenum < n_singlu), in is correctly projected to [samplenum × original_cols], but U/S/vT are still allocated using n_singlu = min(original_rows, original_cols) rather than the projected dimensions:

n_singlu = max(1, min(Tin.shape()[0], Tin.shape()[1]))  // based on ORIGINAL dims
// ... in = Matmul(Q†, in)  → in is now [samplenum × original_cols]
S.Init({n_singlu}, ...)                      // should be {samplenum}
U.Init({in.shape()[0], n_singlu}, ...)        // should be {samplenum, samplenum}
vT.Init({n_singlu, in.shape()[1]}, ...)       // should be {samplenum, original_cols}

---

Critical Issue #2: Block/BlockFermionic paths dead code

RESOLVED — Rsvd_Block_UT_internal is fully implemented for both Block and BlockFermionic types.

Critical Issue #3: Missing semicolon after cytnx_error_msg

RESOLVED — The cytnx_error_msg(true, ...) stub in Gesvd_truncate.cpp is gone.

Critical Issue #4: Helper functions not in unnamed namespaces

RESOLVED — All files (Gesvd.cpp, Gesvd_truncate.cpp, Svd.cpp, Svd_truncate.cpp, Rsvd.cpp) use namespace { // actual implementations: } for internal helpers.

---

Bottom line: Issues #2–4 are all resolved. The cuQuantum fix (issue #1) still needs to be re-applied before merging.