Skip to content

Fix map_eigvals for fermionic case and test #250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Aug 7, 2025
Merged

Conversation

emstoudenmire
Copy link
Contributor

Along similar lines to the recent PR in ITensors.jl, this PR fixes the map_eigvals function in ITensorNetworks.ITensorsExtensions to handle the case of fermionic tensors correctly. The PR includes tests of the case where the non-linear map is the function sqrt, checking that the resulting tensors square back to the original one.

Copy link

codecov bot commented Aug 2, 2025

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.42%. Comparing base (c05a548) to head (e99019f).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/lib/ITensorsExtensions/src/itensor.jl 90.90% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            main     #250       +/-   ##
==========================================
+ Coverage   0.00%   77.42%   +77.42%     
==========================================
  Files         75       77        +2     
  Lines       3784     3823       +39     
==========================================
+ Hits           0     2960     +2960     
+ Misses      3784      863     -2921     
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.

@emstoudenmire
Copy link
Contributor Author

Not sure why those codecov tests are failing since I put in a test of these code pathways. But mainly these changes are quite similar to those in the recent ITensors.jl PR.

@mtfishman mtfishman closed this Aug 5, 2025
@mtfishman mtfishman reopened this Aug 5, 2025
@mtfishman
Copy link
Member

mtfishman commented Aug 5, 2025

@emstoudenmire I noticed in #248 some TEBD tests are failing in this arrow direction check: https://github.com/ITensor/ITensors.jl/blob/c019c65a0f3cb4d7a88e57714bbcfa3986f56de4/src/tensor_operations/matrix_algebra.jl#L79-L84 introduced in ITensor/ITensors.jl#1661.

I think the issue might be that in some of the tests, the auto fermion system is enabled but a non-QN calculation is being run, and those checks are assuming the tensors have QNs.

EDIT: Tests in this PR are failing now too after rerunning them, they were passing before since ITensor/ITensors.jl#1661 wasn't registered.

@mtfishman mtfishman closed this Aug 6, 2025
@mtfishman mtfishman reopened this Aug 6, 2025
@emstoudenmire
Copy link
Contributor Author

Latest updates to this PR include changing the code to be more similar to the final version of the recent ITensors exp PR, and including a test of map_eigvals for a bosonic tensor with the auto fermion system turned on.

You may want to have a look at lines 68 and 69 of the itensor.jl file (first two lines of map_eigvals where I found that the rest of the code had trouble handing the case where Linds and Rinds weren't collections (vectors or tuples) but were individual indices. This made later code like calling reverse(Rinds)... not work correctly. I thought calling collect on Linds and Rinds might be the solution but for an individual Index i, I think collect(i) tries to iterate the Index so doesn't have the same effect as writing [i].

@emstoudenmire
Copy link
Contributor Author

Some of the other tests, such as in test_apply.jl in some places, are using the map_eigvals function with individual Index objects passed as the Linds,Rinds arguments instead of collections. So this stricter behavior is looking more like a breaking change. Of course we are allowing breaking changes in ITensorNetworks, but in hindsight should I just put lines like:

Linds = indices(Linds)
Rinds = indices(Rinds)

at the top of map_eigvals to allow broader types of inputs like individual indices? Or do we want to move these functions to being more "expert" interfaces with narrower allowed inputs?

@mtfishman
Copy link
Member

Some of the other tests, such as in test_apply.jl in some places, are using the map_eigvals function with individual Index objects passed as the Linds,Rinds arguments instead of collections. So this stricter behavior is looking more like a breaking change. Of course we are allowing breaking changes in ITensorNetworks, but in hindsight should I just put lines like:

Linds = indices(Linds)
Rinds = indices(Rinds)

at the top of map_eigvals to allow broader types of inputs like individual indices? Or do we want to move these functions to being more "expert" interfaces with narrower allowed inputs?

I see, thanks for investigating this. I forgot that eigen supports individual Index inputs and canonicalizes in that way, let's just do that here to match the behavior and design of eigen.

@emstoudenmire
Copy link
Contributor Author

Made that change.

As a future pair of PR's, I could move the make_bosonic function into ITensors.jl (like in fermions.jl) to use it in exp and other functions. I don't think Joey and Antonio are going to use these new features in the next day or two so we could even do that change now. (Probably would have to delete and remake this PR here but that would be easy.)

@mtfishman
Copy link
Member

mtfishman commented Aug 7, 2025

Made that change.

As a future pair of PR's, I could move the make_bosonic function into ITensors.jl (like in fermions.jl) to use it in exp and other functions. I don't think Joey and Antonio are going to use these new features in the next day or two so we could even do that change now. (Probably would have to delete and remake this PR here but that would be easy.)

I would prefer to just have repeated definitions of make_bosonic in ITensors.jl and ITensorNetworks.jl to avoid making that function a "public" part of the ITensors.jl interface, since it is a pretty technical and internal function anyway.

@mtfishman mtfishman merged commit d15793c into main Aug 7, 2025
14 checks passed
@mtfishman mtfishman deleted the fermion_map_eigvals branch August 7, 2025 18:32
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.

2 participants