Skip to content

Conversation

lkdvos
Copy link
Contributor

@lkdvos lkdvos commented Sep 21, 2025

This PR fixes #61, since the actual issue isn't flux conservation, the multiplied matrices are just not compatible, and the multiplication should error before even computing the result.

Copy link

codecov bot commented Sep 21, 2025

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (bf41a6c) to head (bd71b85).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/gradedarray.jl 0.00% 4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (bf41a6c) and HEAD (bd71b85). Click for more details.

HEAD has 11 uploads less than BASE
Flag BASE (bf41a6c) HEAD (bd71b85)
docs 6 1
6 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #65       +/-   ##
==========================================
- Coverage   93.14%   0.00%   -93.15%     
==========================================
  Files          20      19        -1     
  Lines         832     823        -9     
==========================================
- Hits          775       0      -775     
- Misses         57     823      +766     
Flag Coverage Δ
docs 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@lkdvos
Copy link
Contributor Author

lkdvos commented Sep 21, 2025

The polar decompositions are failing because the current implementation is fundamentally incompatible with non-zero flux: we have no guarantee that U, S, Vh = tsvd(A) yields U and Vh that can be multiplied. (This was previously also the case as the tests are marked broken, but now the call already errors) I'd suggest to for now comment out these tests, merge this, and I'll fix this in #62 ?

Edit: the additional contraction failures are unrelated to this PR, and main also doesn't pass the tests right now: https://github.com/ITensor/GradedArrays.jl/actions/runs/17896032720

@mtfishman
Copy link
Member

mtfishman commented Sep 22, 2025

Thanks for investigating this. I didn't notice that in the example in #61 the sectors don't match, I agree that case should error earlier based on check_mul_axes.

I also see how polar decomposition of equivariant tensors is nontrivial to define since you need S to be properly square in order to form the isometry U * V. In the current implementation, is the issue that for equivariant tensors we make S block diagonal, which means in general it won't be square (i.e. the sectors in the codomain and domain of S might be permuted from each other)? Would a solution be that for the sake of the polar decomposition we gauge transform S so that it is square (or in general for thin SVD, make S square even if that means that sometimes it isn't block diagonal)?

I'm fine with merging this PR as it is and investigating these other issues in followup PRs. That's too bad about the test failures, I wonder how that slipped through the downstream testing... Maybe that was caused by the refactor of the contraction code in ITensor/TensorAlgebra.jl#75.

@mtfishman mtfishman merged commit 3a057e4 into main Sep 22, 2025
10 of 18 checks passed
@mtfishman mtfishman deleted the gradedrange branch September 22, 2025 15:20
@lkdvos
Copy link
Contributor Author

lkdvos commented Sep 22, 2025

I indeed think we can fix the polar decomposition by making the right choices of the gauges, in particular not putting the flux on the S should be enough I think. I am tackling that in the other PR, I'll try and push my update once I'm past border control ;)

@mtfishman
Copy link
Member

I'm fine with merging this PR as it is and investigating these other issues in followup PRs. That's too bad about the test failures, I wonder how that slipped through the downstream testing... Maybe that was caused by the refactor of the contraction code in ITensor/TensorAlgebra.jl#75.

I see, it appears the GradedArrays.jl isn't a downstream test of TensorAlgebra.jl, I'll fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Flux conservation broken with gradedranges that have zero-length parts

2 participants