Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #732 +/- ##
==========================================
+ Coverage 27.90% 29.58% +1.67%
==========================================
Files 241 213 -28
Lines 35470 30474 -4996
Branches 14778 12012 -2766
==========================================
- Hits 9898 9015 -883
+ Misses 18591 16656 -1935
+ Partials 6981 4803 -2178
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ca1b01b1d
ℹ️ 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".
| } | ||
| std::vector<Tensor> blks; | ||
| // std::cout<<out[1]; | ||
| algo::Vsplit_(blks, out[1], split_dims); |
There was a problem hiding this comment.
The fermionic reconstruction always does Vsplit_ on out[1] using row-block sizes, which only matches the column-vector layout. In the !row_v branch of tensor Eig, eigenvectors are returned in row form, so splitting on rows assigns qindices/reshapes along the wrong axis and produces incorrect eigenvector block structure (this affects the default row_v value). The split/remap logic needs to branch on row_v (and the same issue appears in the copied fermionic Eigh code).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a bug in Eig and Eigh that already existed for BlockUniTensor. I created a new issue for it, see #782. For now, I throw an error message if !row_v.
|
How about to ask AI to create some tests which also cover the problem that AI pointed out? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7deb36534
ℹ️ 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".
…rmionic)UniTensor see #782
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 900ee1a9f4
ℹ️ 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".
pcchen
left a comment
pcchen
left a comment
There was a problem hiding this comment.
Posted by Claude Code on behalf of @pcchen
PR #732 Review: Linear algebra functions for fermions
Overview
This PR implements Eig, Eigh, Gesvd_truncate, and Qr for BlockFermionicUniTensor, completing fermionic support for these core linalg operations. It also includes widespread cleanup across all linalg files: removing using namespace std, renaming internal helpers to static functions with _internal suffixes, removing split namespace blocks, and improving error messages uniformly.
Correctness
Core fermionic logic (sign-flip handling):
- The sign-flip approach in all four new
_BlockFermionic_UT_internalfunctions is consistent: blocks withsignflip[b] == trueare negated (Tlist[i] = -Tlist[i]) before being assembled into the big block matrix. This matches the pattern established in the existingGesvdimplementation. The approach is correct for Hermitian/symmetric matrices (Eigh) since negating a block still yields a Hermitian block. - For general
Eig(non-Hermitian case), negating a row-block before assembly is also correct.
Output _signflip always set to false:
- In
Eig_BlockFermionic_UT_internal,Eigh_BlockFermionic_UT_internal, andQr_BlockFermionic_UT_internal, the output eigenvector/Q/R matrices are created with_signflip = std::vector(v_blocks.size(), false). This is correct because the sign has already been absorbed into the numerical values, so the returned UniTensors carry no pending sign-flip.
Potential issue — Eig with is_V=true, column index into v_itoi:
In Eig_BlockFermionic_UT_internal, the is_V branch does:
v_itoi.back()[Tin.rowrank()] = e_blocks.size() - 1;
v_itoi.back().resize(Tin.rowrank() + 1);v_itoi.back() is populated from Tin.get_qindices(...), which has Tin.rank() elements. The assignment [Tin.rowrank()] sets the aux-bond index, and resize truncates to rowrank + 1. This mirrors Eig_Block_UT_internal exactly, so it appears correct — but worth verifying that Tin.get_qindices(b) returns a copy (not a reference that would mutate the original tensor's index table).
Gesvd_truncate: merging Block and BlockFermionic paths:
- The old
_gesvd_truncate_BlockFermionic_UTand_gesvd_truncate_Block_UTare replaced by a singleGesvd_truncate_Block_UTs_internalthat handles both by checkingTin.uten_type() == UTenType.BlockFermionicwhen callingvec_erase_(...signflip_...). This is clean and reduces duplication significantly. - The
to_be_removed.empty()guard added around allvec_erase_calls is a genuine bug fix — previously this would callvec_erase_with an empty list on every pass.
Code Quality
Positive changes:
- Removing
using namespace stdand replacing withstd::qualifiers throughout — good hygiene. - Renaming
_Eig_Dense_UT→Eig_Dense_UT_internal(withstatic) is better practice than the leading-underscore naming convention. - Collapsing double
namespace cytnx { namespace linalg { ... } }patterns into single nested blocks. - Improving error messages with actual values (
rank is %d) rather than just descriptions.
Issues and suggestions:
-
Significant code duplication.
Eig_BlockFermionic_UT_internalandEigh_BlockFermionic_UT_internalare near-identical; the only difference is the call tolinalg::Eigvslinalg::Eigh. Similarly,Qr_BlockFermionic_UT_internalduplicates the block-combination boilerplate. The PR does add "Exactly the same as ... with Eig() → Eigh()" comments which is helpful, but the duplicated code is a maintenance burden going forward. -
Leftover commented-out debug code. Several places retain commented-out
std::coutstatements and dead code:Eig.cpp/Eigh.cpp:// std::cout << BdLeft << std::endl;,// std::cout << strides << std::endl;, etc.Eig.cppis_Vblock:// std::cout << out[tr].shape() << std::endl;— notetrisn't declared in that scope (copy-paste artifact fromGesvd), harmless since it's in a comment but worth removing.- Commented-out alternatives like
// Tlist[i] = Tlist[i].Mul(-1);and stubs like// vec2d<cytnx_uint64> vT_itoi;should be removed.
-
Rsvd_truncate.cppresidual};:}; // Rsvd_truncate_Dense_UT_internalThe trailing semicolon after
}is harmless but inconsistent with how all other functions were cleaned up in this PR. Minor. -
resizeafter assignment inv_itoi: In bothEigandEighfermionic internals,resizeis called after writing tov_itoi.back()[Tin.rowrank()]. This is correct sincerowrank < rankis validated, but the order (write then truncate) is slightly fragile. Resizing first and then assigning would be clearer.
Test Coverage
- Two new error-case tests (
err_rowrank_equal_rank_dense_UT) added forSvdandGesvd— good. - Two new
Eig/Eighrow-v tests added inlinalg_test.cpp. - Missing: No tests exercise the new fermionic code paths (
Eig_BlockFermionic_UT_internal,Eigh_BlockFermionic_UT_internal,Qr_BlockFermionic_UT_internal) directly. Given the complexity of sign-flip handling, at minimum one round-trip correctness test for each new fermionic function would be valuable before merge.
Summary
The core logic is sound and the cleanup is a genuine improvement. Main asks before merge:
- Remove dead/commented-out debug code.
- Add at least basic fermionic round-trip tests for the newly implemented functions.
- (Minor) Fix the residual
};inRsvd_truncate.cpp.
|
Cleaned up the code and included suggestions. Could someone use AI support to write unit tests? |
|
You can probably directly ask @codex to write the tests? |
Summary
Testing
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6677eb80e3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f0e8fbe11
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 765173a083
ℹ️ 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".
…tions of linalg functions into one template to avoid boilerplate
Issue Status CheckPosted by Claude Code on behalf of @pcchen Here's the current status of each raised issue: Issue 1 — P1: Guard BlockFermionic Eig against missing column bondRESOLVED. Fixed in commit Issue 2 — P1: Respect
|
|
@manuschneider Can you double check if you have fix Issue 6 (complex dtype). I will approve and merge after you verify this. |
The I changed the So this is resolved now. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3217bd69f5
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef94d4b8fe
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8590b4937f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 023eaf3dc4
ℹ️ 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".
| "[ERROR][Svd] Input UniTensor should have rowrank>0, but rowrank is %d\n", | ||
| Tin.rowrank()); | ||
| cytnx_error_msg( | ||
| Tin.rowrank() >= Tin.rank(), |
There was a problem hiding this comment.
Because this validation runs before dispatching on the UniTensor type, dense UniTensors with rowrank == rank now fail even though the previous dense path supported them by reshaping the data to a (prod(row dims) x 1) matrix and calling the Tensor SVD. This is a user-visible regression for dense tensors whose rowrank intentionally places all physical legs on the row side; the new rowrank < rank guard is needed for the block implementation's column-bond access, but should not reject the dense case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
If rowrank == rank, the return is U = normalized vector, S norm of vector, V trivial one-element tensor.
I do not think that users should call SVD to normalize a vector. This was possible only in the dense branch. If we allow it, we should do this everywhere. But I would keep it as is, because running SVD on a vector instead of a matrix is most likely not intended by the user; it's more likely a bug in their code logic. And if it is intended, it can easily be replaced by dividing by the norm (which is cleaner code as well).
|
All linear algebra functions that previously accepted either This PR is ready for a final review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 546a3f4566
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 229f7a805d
ℹ️ 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".
Issue Status CheckPosted by Claude Code on behalf of @pcchen Here is the current status of all raised issues: P1 Issues
P2 Issues
All issues are addressed. This PR is approved for merge. |
4 participants
All linear algebra functions that accept
BlockUniTensororLinOpwork with fermions. This affects:Eig,Eigh,Gesvd_truncate,Qr,ExpH,ExpM,Arnoldi,Lanczos,Lanczos_Gnd_Ut.