[ITensors] ITensorMPS submodule

[emstoudenmire]

Description

Creates a new ITensorMPS module inside of the ITensors module. The user experience should be unchanged, since all exports are kept the same in ITensors i.e. re-exporting the ITensorMPS exports. Most of the changes in this PR besides moving files and renaming the mps/ folder to the ITensorMPS/ folder are:

• introduce ITensorMPS module as discussed above
• add imports.jl and exports.jl for this module (most of the work was getting the correct imports figured out)
• moved all of physics/autompo/ into the ITensorMPS folder and module, since it is really a system for creating MPOs from OpSum data
• minor modifications to some tests which used unexported types and methods to get them from the ITensors.ITensorMPS module now

How Has This Been Tested?

Checked that ITensorLegacyMPS/base tests are passing and ITensors base tests are passing on my machine.

Checklist:

• My code follows the style guidelines of this project. Please run using JuliaFormatter; format(".") in the base directory of the repository (~/.julia/dev/ITensors) to format your code according to our style guidelines.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have added tests that verify the behavior of the changes I made.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.

[emstoudenmire]

One remaining thing to be discussed is whether the precompile related code in ITensors that involves types like MPS and MPO should be moved into the ITensorMPS module or not.

[codecov-commenter]

Codecov Report

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

Project coverage is 53.45%. Comparing base (093d339) to head (54a636b).
Report is 1 commits behind head on main.

❗ Current head 54a636b differs from pull request most recent head 63cbda7. Consider uploading reports for the commit 63cbda7 to get more accurate results

Files	Patch %	Lines
src/ITensorMPS/abstractmps.jl	0.00%	4 Missing ⚠️
src/ITensorMPS/autompo/opsum_to_mpo_generic.jl	0.00%	4 Missing ⚠️
src/ITensorMPS/autompo/opsum_to_mpo_qn.jl	0.00%	4 Missing ⚠️
src/ITensorMPS/mpo.jl	0.00%	4 Missing ⚠️
...TensorsNamedDimsArraysExt/src/to_nameddimsarray.jl	0.00%	1 Missing ⚠️

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

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1356       +/-   ##
===========================================
- Coverage   84.40%   53.45%   -30.96%     
===========================================
  Files         100       99        -1     
  Lines        8581     8452      -129     
===========================================
- Hits         7243     4518     -2725     
- Misses       1338     3934     +2596     

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

[mtfishman]

Thanks @emstoudenmire. Perhaps we should change test/ITensorLegacyMPS to test/ITensorMPS.

[mtfishman]

Thanks @emstoudenmire, this is a nice change to make sure we keep the MPS code self-contained, and also more clearly separated from things like operator definitions.

Something else we might want to do is move ITensorMPS and submodules like src/ContractionSequenceOptimization, src/ITensorChainRules, src/Ops, etc. into a file src/lib. That is the organization we have in NDTensors (https://github.com/ITensor/ITensors.jl/tree/v0.3.57/NDTensors/src/lib) which I think has helped to keep things organized.

I even went with organizing those files as projects themselves, i.e. in this case we could have a file organization:

src/lib/ITensorMPS/src/ITensorMPS.jl
src/lib/ITensorMPS/src/# Files making up the `ITensorMPS` module
src/lib/ITensorMPS/test/runtest.jl
src/lib/ITensorMPS/test/# Test files, the ones currently in `test/ITensorLegacyMPS`

Then, the directory src/lib/ITensorMPS will already be organized in a way that it can be moved to a separate repository, once we are ready to do that.

Maybe we don't need to do that all in this PR, let's discuss the logistics.

[mtfishman]

One remaining thing to be discussed is whether the precompile related code in ITensors that involves types like MPS and MPO should be moved into the ITensorMPS module or not.

Do you mean code in src/packagecompile/ or in precompile/?

EDIT: precompile/ isn't really being used in practice anyway (we should probably just remove it to not cause confusion about that), so the only relevant part for this discussion is the code in src/packagecompile/. It's a good question where that should go. I think probably it should go into the ITensorMPS module, since in the future that version of the compile() should likely get moved to the ITensorMPS package, since it is really compiling DMRG.

[emstoudenmire]

I made the above changes such as renaming ITensorLegacyMPS/ to ITensorMPS/ and the review comments on the code.
I'd be happy to move the src/packagecompile/ code and remove the precompile/ too but maybe better in a different PR? What do you think?

Also I like the idea of going ahead with the lib folder and making ITensorMPS/ have a package structure (or even be an actual package already). Should we go for it in this PR? Maybe in another to make each PR more "atomic"?

[emstoudenmire]

I'm all for these changes to best practices around imports and exports. I remember that I think even a few years ago we had actually discussed the possibility of not using imports and just qualifying things i.e. ModuleName.function instead, which I like since it's very "self documenting". So I'm supportive of that.

The other ideas about how to apply using and about modules for tests are new ones for me, and sound like really good ideas. I agree we should be a lot more sparing in the future both about what we import and export at all levels and in all packages. But makes sense that for ITensorMPS we are a bit constrained by backwards compatibility.

[emstoudenmire]

The only outstanding issue is about import / export of the contract method in the ITensorMPS module. Here's my current chain of reasoning about it:

1. Both ITensorMPS and ITensors export contract. This is because e.g. ITensors defines contract of ITensor objects and ITensorMPS defines contract of MPO objects among other overloads.
2. ITensorMPS depends on ITensors
3. So ITensorMPS should either: 3a) do import ..ITensors: contract or 3b) qualify implementations of contract as ITensors.contract.

Currently the approach is to use import ..ITensors: contract in ITensorMPS. I'm fine with changing to the other approach. But I currently don't see an alternative to doing one of those.

Or let me know if your comment above was about something else (e.g. there's a reason that ITensorMPS should not be exporting contract?).

[mtfishman]

The only outstanding issue is about import / export of the contract method in the ITensorMPS module. Here's my current chain of reasoning about it:

1. Both `ITensorMPS` and `ITensors` export `contract`. This is because e.g. `ITensors` defines `contract` of `ITensor` objects and `ITensorMPS` defines `contract` of `MPO` objects among other overloads.

2. `ITensorMPS` depends on `ITensors`

3. So `ITensorMPS` should either: 3a) do `import ..ITensors: contract` or 3b) qualify implementations of contract as `ITensors.contract`.

Currently the approach is to use import ..ITensors: contract in ITensorMPS. I'm fine with changing to the other approach. But I currently don't see an alternative to doing one of those.

Or let me know if your comment above was about something else (e.g. there's a reason that ITensorMPS should not be exporting contract?).

I think part of the confusion was that I didn't understand the issue you were describing around the test failing.

I would vote for:

1. ITensorMPS overloading ITensors.contract, either with explicit ITensors.contract in the function definition (which is the preferred syntax now) or with import ..ITensors: contract. I'll leave that up to you to decide between the two styles for the sake of this PR, probably we'll want to update that style everywhere so it may not be worth doing here since that is a bigger undertaking, but chipping away little by little in PRs like this could be nice to do.
2. I would vote for ITensorMPS not exporting contract, or any other function that is originally defined in ITensors and already exported by ITensors. If users want all of the functions exported by both ITensorMPS and ITensors, they can do using ITensors; using ITensorMPS at the top of their file.
3. In terms of the style in the tests for making use of contract, as I said, my new approach for that is to explicitly put using M: f to use a function f from module M. If f exists in multiple modules (say it is overloaded by a module N that depends on M), I try my best to include f from the module where it was originally defined. That means, in the tests, we should use using ITensors: contract, even if we are using the contract function on MPS and MPO objects.

[mtfishman]

So the issue was that contract wasn't being exported from ITensors?

[mtfishman]

@emstoudenmire it looks like the GPU tests are failing because they are trying to run ITensorGPU/test/test_cuiterativesolvers.jl, which has now been removed.

[emstoudenmire]

Yeah! Pretty embarassing but that was the issue (i.e. contract not being exported). The contract issue turned out to be a combination of a silly mistake on my part plus the confusion I had about import/export which I've now cleared up thanks to our discussion. All it was was that I had accidentally deleted the export of contract from ITensors and then didn't understand what the error message was really saying.

I've now restored the export of contract from ITensors and removed the export of contract from ITensorMPS.

One last thing to do is to remove the import of contract and just write ITensors.contract instead. I don't mind going through and doing that because like you said we want to start moving toward this in chunks of work.

[emstoudenmire]

I've now also removed the entry for the deleted ITensorGPU test

[mtfishman]

One last thing to do is to remove the import of contract and just write ITensors.contract instead. I don't mind going through and doing that because like you said we want to start moving toward this in chunks of work.

Sounds good. After that it looks good to merge from my end. Unfortunately we are importing a lot of functions right now, so it will be quite an undertaking to change over the style everywhere. This may be a case where it is worth trying to automate that process (I need to change over other repositories as well).