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] Added option for MPO-MPS zipup #1536

Closed
wants to merge 10 commits into from

Conversation

corbett5
Copy link
Contributor

@corbett5 corbett5 commented Oct 8, 2024

Description

Minimal changes necessary to extend the "zipup" algorithm for MPO-MPO products to MPO-MPS products. The main request I foresee is to bring it inline with the "naive" algorithm which also has a single implementation for MPO-MPO(S) products. Once those details are worked out I'll add docs and tests.

How Has This Been Tested?

It has not.

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.

@corbett5 corbett5 changed the title Added option for MPO-MPS zipup. [ITensorMPS] Added option for MPO-MPS zipup. Oct 8, 2024
@mtfishman
Copy link
Member

Looks good, thanks @corbett5. Could you add a test for the new functionality? Also could you add something about truncate_kwargs in the docstring?

@corbett5
Copy link
Contributor Author

@mtfishman I updated the docs and tests, but I seem to be having trouble with the formatting. For one the formatter updated 100+ files which I did not touch, but I also can't get the format check to pass. It passes for me locally with the same JuliaFormatter version (v2.0.0).

Also now none of the other tests pass after the extensive format changes.

@mtfishman
Copy link
Member

mtfishman commented Oct 17, 2024

@corbett5 it looks like JuliaFormatter.jl v2 either accidentally or deliberately changed certain formatting rules, including introducing some bugs into the code by erroneously changing positional arguments to keyword arguments.

Please format the code using JuliaFormatter.jl v1 (i.e. revert to the previous formatting), and I can change the formatting CI checks to use JuliaFormatter.jl v1 for now while those issues get worked out.

@corbett5 corbett5 force-pushed the feature/corbett/mpo-mps-zipup branch from 3877c20 to e2979a1 Compare October 17, 2024 17:54
@corbett5
Copy link
Contributor Author

All passing 🎉

Thanks for fixing the formatting problems.

@mtfishman
Copy link
Member

mtfishman commented Oct 17, 2024

I actually didn't do anything, it looks like the JuliaFormatter.jl v2 release was reverted ("yanked" in the Julia registry parlance) because there were too many bugs (domluna/JuliaFormatter.jl#878).

@mtfishman mtfishman changed the title [ITensorMPS] Added option for MPO-MPS zipup. [ITensorMPS] Added option for MPO-MPS zipup Oct 20, 2024
Comment on lines 959 to 967
- `alg="zipup"`: the algorithm to use for the contraction. Supported algorithms are
- "naive": The MPO tensors are contracted exactly at each site and then a truncation
of the resulting MPO is performed.
- "zipup": The MPO and MPS tensors are contracted then truncated at each site without enforcing
the appropriate orthogonal gauge. Once this sweep is complete a call to `truncate!` occurs.
Because the initial truncation is not locally optimal it is recommended to use a loose
`cutoff` and `maxdim` and then pass the desired truncation parameters to the locally optimal
`truncate!` sweep via the additional keyword argument `truncate_kwargs`.
Suggested use is `contract(A, ψ; method="zipup", cutoff=cutoff / 10, maxdim=2 * maxdim, truncate_kwargs=(; cutoff, maxdim))`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that we are repeating the same docstring in two places.

Can we factorize out the parts that are common to contract and apply in a new string contract_or_apply_mpo_mpo_doc that is used by both of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pulled out the zipup docs, but I could do more to bring them in line if you want.

@mtfishman
Copy link
Member

@corbett5 could you move this PR over to: https://github.com/ITensor/ITensorMPS.jl?

@mtfishman
Copy link
Member

I'll close this since it needs to be moved to the ITensorMPS.jl repository.

@mtfishman mtfishman closed this Nov 19, 2024
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