Skip to content
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

Refactor sqrt_inv_sqrt #158

Merged
merged 25 commits into from
Apr 22, 2024
Merged

Refactor sqrt_inv_sqrt #158

merged 25 commits into from
Apr 22, 2024

Conversation

JoeyT1994
Copy link
Contributor

This PR removes the sqrt_inv_sqrt function in apply.jl in favor of a map_itensor(f::Function, t::ITensors, args...) added in the file itensorsextensions.jl in the new internal module ITensorsExtensions (where src/ITensorsExt/utils.jl is also now moved).

map_itensor(f::Function, A::ITensor, ...) uses the SVD to take a decomposition U, S, V of A and applies f over the diagonal elements of S. This uses the sqrt_decomp function from ITensors.jl (introduced in the PR https://github.com/ITensor/ITensors.jl/pull/1197/files) which correctly handles quantum numbers. The indices of the returned ITensor match those of the input A.

The symmetric_factorize function has been removed entirely from apply.jl as we can just call factorize_svd(A::ITensor; ortho = "none") from ITensors.jl` to do the same thing (again this was introduced in PR https://github.com/ITensor/ITensors.jl/pull/1197/files).

Tests are included. @mtfishman these appear to show the square root / inverse being correctly taken by the SVD backend and I don't see any phase issues cropping up?

A few other changes in this PR:
src/tensornetworkoperators.jl and test/test_tnos.jl have been removed and archived as they will be significantly refactored down the line.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 65.51724% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 82.66%. Comparing base (ae4ad2c) to head (0dc6016).
Report is 4 commits behind head on main.

Files Patch % Lines
src/apply.jl 52.38% 10 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
+ Coverage   81.89%   82.66%   +0.77%     
==========================================
  Files          72       72              
  Lines        3744     3635     -109     
==========================================
- Hits         3066     3005      -61     
+ Misses        678      630      -48     

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

@mtfishman
Copy link
Member

mtfishman commented Apr 14, 2024

I have to say that it doesn't sit well with me to use an SVD to perform square roots and inverses of positive semi-definite Hermitian matrices, I think it is safer and more direct (not to mention faster) to use a Hermitian eigendecomposition to ensure the gauge is fixed in the correct way.

@mtfishman mtfishman changed the title Refactor Sqrt inv sqrt Refactor sqrt_inv_sqrt Apr 15, 2024
JoeyT1994 and others added 7 commits April 15, 2024 13:26
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
src/apply.jl Outdated Show resolved Hide resolved
src/apply.jl Outdated Show resolved Hide resolved
JoeyT1994 and others added 2 commits April 22, 2024 08:39
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
Co-authored-by: Matt Fishman <mtfishman@users.noreply.github.com>
@mtfishman
Copy link
Member

Looks good, thanks! Good to go back and start cleaning up some of this code.

@mtfishman mtfishman merged commit 184e2e1 into ITensor:main Apr 22, 2024
5 of 9 checks passed
@JoeyT1994 JoeyT1994 deleted the sqrt_inv_sqrt branch June 16, 2024 13:46
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.

3 participants