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

[ITensorMPS] Fix some lingering namespace issues #1362

Merged
merged 6 commits into from
Mar 25, 2024
Merged

Conversation

emstoudenmire
Copy link
Collaborator

Fixes of test errors in test/ITensorMPS/Ops/ and the ITensorGaussianMPS package.

Also attempting to fix the error seen when running the ITensorGPU tests.

@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2024

Codecov Report

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

Project coverage is 53.45%. Comparing base (2567204) to head (361de4e).

❗ Current head 361de4e differs from pull request most recent head de5f118. Consider uploading reports for the commit de5f118 to get more accurate results

Files Patch % Lines
src/ITensorMPS/abstractprojmpo/projmposum.jl 0.00% 3 Missing ⚠️
src/ITensorMPS/dmrg.jl 0.00% 3 Missing ⚠️

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

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1362       +/-   ##
===========================================
- Coverage   84.13%   53.45%   -30.68%     
===========================================
  Files         100       99        -1     
  Lines        8527     8452       -75     
===========================================
- Hits         7174     4518     -2656     
- Misses       1353     3934     +2581     

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

@emstoudenmire
Copy link
Collaborator Author

[test ITensors mps]

@emstoudenmire
Copy link
Collaborator Author

[test ITensorGaussianMPS]

Copy link
Contributor

Run ITensorGaussianMPS tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8415394326

1 similar comment
Copy link
Contributor

Run ITensorGaussianMPS tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8415394326

Copy link
Contributor

Run ITensors mps tests from comment trigger: failed ❌
https://github.com/ITensor/ITensors.jl/actions/runs/8415370522

1 similar comment
Copy link
Contributor

Run ITensors mps tests from comment trigger: failed ❌
https://github.com/ITensor/ITensors.jl/actions/runs/8415370522

@emstoudenmire
Copy link
Collaborator Author

So the test failure here for the MPS tests is strange: apparently allequal was not defined in Base until Julia 1.8. But it is used in some @assert's in the file abstractprojmpo/projmposum.jl. So I'm surprised these tests were ever passing.

The question is whether we should:

  1. replace the use of allequal with something else there or
  2. raise the minimum Julia version to 1.8

I'd be fine with either but I think for now better to just replace allequal if we are just using it in a small number of places.

@emstoudenmire
Copy link
Collaborator Author

[test ITensors mps]

@emstoudenmire
Copy link
Collaborator Author

[test ITensorGaussianMPS]

Copy link
Contributor

Run ITensorGaussianMPS tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8416208821

1 similar comment
Copy link
Contributor

Run ITensorGaussianMPS tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8416208821

Copy link
Contributor

Run ITensors mps tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8416207106

1 similar comment
Copy link
Contributor

Run ITensors mps tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8416207106

@emstoudenmire
Copy link
Collaborator Author

I went ahead and replaced allequal but let's discuss which choice would be best.

@mtfishman
Copy link
Member

So the test failure here for the MPS tests is strange: apparently allequal was not defined in Base until Julia 1.8. But it is used in some @assert's in the file abstractprojmpo/projmposum.jl. So I'm surprised these tests were ever passing.

The question is whether we should:

1. replace the use of `allequal` with something else there or

2. raise the minimum Julia version to 1.8

I'd be fine with either but I think for now better to just replace allequal if we are just using it in a small number of places.

I've been using the Compat.jl package to address issues like that. Compat.jl makes some subset of newer Julia functionality (like allequal) available in older versions of Julia. Probably it was working before because we load Compat in ITensors.jl, but it isn't being loaded in the ITensorMPS module now. So we could add using Compat: allequal at the top of the abstractprojmpo/projmposum.jl file.

@mtfishman
Copy link
Member

[test ITensors mps]

We should change the trigger comment for this workflow to [test ITensorMPS], it just requires changing this line: https://github.com/ITensor/ITensors.jl/blob/v0.3.57/.github/workflows/comment_trigger_test_itensors_mps.yml#L12.

Copy link
Contributor

Run ITensors mps tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8420084779

1 similar comment
Copy link
Contributor

Run ITensors mps tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8420084779

@mtfishman mtfishman changed the title Fixes related to ITensorMPS module [ITensorMPS] Fix some lingering namespace issues Mar 25, 2024
src/ITensorMPS/dmrg.jl Outdated Show resolved Hide resolved
@emstoudenmire
Copy link
Collaborator Author

[test ITensors mps]

@emstoudenmire
Copy link
Collaborator Author

[test ITensorGaussianMPS]

Copy link
Contributor

Run ITensorGaussianMPS tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8421425459

1 similar comment
Copy link
Contributor

Run ITensorGaussianMPS tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8421425459

Copy link
Contributor

Run ITensors mps tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8421423550

1 similar comment
Copy link
Contributor

Run ITensors mps tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8421423550

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