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

[ITensors] Tests for general fermionic H #1200

Merged
merged 3 commits into from
Oct 31, 2023
Merged

[ITensors] Tests for general fermionic H #1200

merged 3 commits into from
Oct 31, 2023

Conversation

emstoudenmire
Copy link
Collaborator

We have not previously had a test for a very general type of fermionic Hamiltonian. This PR adds tests of a rather general H (actually not even assuming it is Hermitian) for both the usual OpSum system and with the auto fermion system enabled.

@mtfishman
Copy link
Member

mtfishman commented Sep 21, 2023

@emstoudenmire this seems like an unfortunate duplication of code, and also introduces code that involves MPS/MPO into test/base instead of test/ITensorLegacyMPS/base.

Can we combine into one testset in test/ITensorLegacyMPS/base? As we discussed, I think it is an "anti-pattern" to rely on MPS/MPO tests to test core functionality like auto fermions. If we detect an auto fermion failure through an MPS/MPO test, that reflects a failure of not having enough basic unit tests that work with core NDTensor or ITensor objects like Tensor, ITensor, etc. Also as discussed this will help with plans for moving the MPS/MPO code to a separate package once ITensorNetworks.jl is more developed.

@mtfishman
Copy link
Member

Relatedly, it would be nice to move the other fermion tests that make use of MPS/MPO functionality to test/ITensorLegacyMPS/base.

@mtfishman mtfishman changed the title Tests for general fermionic H [ITensors] Tests for general fermionic H Sep 21, 2023
@emstoudenmire
Copy link
Collaborator Author

emstoudenmire commented Sep 23, 2023

Makes sense. I moved all of the MPS, MPO, and OpSum related tests from base/test_fermions.jl into either ITensorLegacyMPS/base/test_autompo.jl or a new file ITensorLegacyMPS/base/test_fermions.jl . It might be better to even move the tests in that new test_fermions.jl file into other existing files, but keeping them all together does help with things like not having to call ITensors.enable_auto_fermions() in multiple places.

I definitely agree overall with the point about not testing AutoFermion too much through higher-level things like MPS algorithms. So probably some of those tests should be deleted eventually. But for now there are a few cases where certain issues, like the MPS(sites,state) product MPS constructor not putting in links with arrows in a very certain way, worked anyhow for every other non-fermionic case and only failed for fermions. However, it wasn't exactly a failure of the AutoFermion system per se, it's more that the new code there is now doing the most mathematically correct thing that would also be needed for cases like SU(2), but you can get away without it in every other situation we have right now (e.g. Abelian symmetries or complex tensors).

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2023

Codecov Report

Patch coverage has no change and project coverage change: -30.86% ⚠️

Comparison is base (f6c575b) 85.41% compared to head (e928999) 54.55%.
Report is 5 commits behind head on main.

❗ Current head e928999 differs from pull request most recent head 9da26e3. Consider uploading reports for the commit 9da26e3 to get more accurate results

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

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1200       +/-   ##
===========================================
- Coverage   85.41%   54.55%   -30.86%     
===========================================
  Files          88       87        -1     
  Lines        8426     8373       -53     
===========================================
- Hits         7197     4568     -2629     
- Misses       1229     3805     +2576     

see 37 files with indirect coverage changes

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

@mtfishman
Copy link
Member

Thanks!

@mtfishman mtfishman merged commit 62f162f into main Oct 31, 2023
7 checks passed
@mtfishman mtfishman deleted the test_chem_H branch October 31, 2023 00:38
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