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.76303% with 554 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.35%. Comparing base (b88a090) to head (37eb8d4).
⚠️ Report is 10 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	65.58%	58 Missing and 69 partials ⚠️
src/linalg/Gesvd_truncate.cpp	60.82%	54 Missing and 51 partials ⚠️
src/linalg/Svd_truncate.cpp	65.89%	46 Missing and 42 partials ⚠️
src/linalg/Gesvd.cpp	90.34%	20 Missing and 14 partials ⚠️
src/linalg/Rsvd_notruncate.cpp	94.25%	6 Missing and 19 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #744      +/-   ##
==========================================
+ Coverage   36.32%   38.35%   +2.02%     
==========================================
  Files         214      213       -1     
  Lines       33143    31171    -1972     
  Branches    12998    12846     -152     
==========================================
- Hits        12040    11956      -84     
+ Misses      19182    17307    -1875     
+ Partials     1921     1908      -13     

☔ 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. to 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.

[manuschneider]

Previous issue on GPU is resolved.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 37eb8d48e2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[ianmccul]

I had a look at this locally, with a bit of AI assistance.

• Rsvd(..., is_U=false, is_vT=true) can segfault. In src/linalg/Rsvd.cpp:62, the implementation calls Gesvd(..., is_U, is_vT) but later assumes tmps[1] and tmps[2] both exist. With V-only output, tmps is [S, vT], so indexing tmps[2] goes out of bounds. I reproduced this for both Tensor and UniTensor dense Rsvd. U-only also evaluates tmps[2], so it has undefined behavior, although my test didn't actually crash in this case.

• C++ defaults oversampling_summand = 10 in include/linalg.hpp:906, while Python bindings default it to 20 in pybind/linalg_py.cpp:103.

• The main dense Rsvd tests in tests/linalg_test/Rsvd_test.cpp:36 mainly exercise default is_U=true, is_vT=true. There should be explicit Tensor and UniTensor tests for S-only, U-only, V-only, and U+V, including return_err.

• I don't understand what Rsvd_notruncate is supposed to do. For a Tensor, it appears to be a randomized SVD with no oversampling and no truncation. I'm not sure what the application of this would be. If you want some approximate singular value space then you could just omit the SVD and it becomes the range-finding algorithm. If you wanted the actual singular value spectrum then its not going to be accurate without oversampling. I guess it might be useful if you wanted to get N approx singular vectors and manage oversampling yourself. Rsvd_notruncate then would appear to be equivalent to Rsvd but with no oversampling, but that is not the case: Rsvd_notruncate always does left-sketching if keepdim < min(m,n), but Rsvd does left-sketching or right-sketching depending on whether m <= n or not (for m x n matrix). I think this is the wrong way around from what is optimal. The idea is that you contract over the larger dimension and do the SVD on the smaller matrix.

• For a UniTensor, Rsvd_notruncate has oversampling parameters, and instead of returning keepdim singular values, it returns the number given by keepdim and the oversampling parameters.

• Restating the point from above, the left/right sketching choice in Rsvd looks backwards for rectangular matrices. If Tin has shape m x n and the sample size is l, the usual cost-reducing choice is to project away the larger external dimension: for m >= n, form Q for the column space and run the small SVD on Q^H A with shape l x n; for n > m, form Q for the row space and run the small SVD on A Q with shape m x l.

  The current implementation does the opposite: when m <= n, it computes Q^H A, leaving the larger n dimension in the SVD; when m > n, it computes A Q^H, leaving the larger m dimension in the SVD. This may still produce a valid approximation, but it appears to defeat the stated purpose of reducing the cost of the SVD for rectangular matrices.