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] Rename most functions exported by ITensorTDVP #1397

Closed
wants to merge 1 commit into from

Conversation

emstoudenmire
Copy link
Collaborator

This PR is a companion to ITensor/ITensorTDVP.jl#67 which renames most functions prepended with itensortdvp_ back to their original name. Only itensortdvp_dmrg is left as-is.

@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 49.89%. Comparing base (ceb26a7) to head (38d1a89).

❗ Current head 38d1a89 differs from pull request most recent head 2f68f41. Consider uploading reports for the commit 2f68f41 to get more accurate results

Files Patch % Lines
src/ITensorMPS/tdvp.jl 0.00% 12 Missing ⚠️
src/ITensorMPS/contract_mpo_mps.jl 0.00% 1 Missing ⚠️
src/ITensorMPS/dmrg_x.jl 0.00% 1 Missing ⚠️
src/ITensorMPS/linsolve.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    #1397      +/-   ##
==========================================
+ Coverage   49.23%   49.89%   +0.66%     
==========================================
  Files         110      113       +3     
  Lines        8320     8822     +502     
==========================================
+ Hits         4096     4402     +306     
- Misses       4224     4420     +196     

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

@mtfishman
Copy link
Member

An issue I see with the latest design is that it overloads ITensors.contract and KrylovKit.linsolve, which means that both ITensors.jl and ITensorTDVP.jl overload those functions with the same signature which will lead to warnings and precompilation failures (Julia's precompilation system breaks if two packages overload the same function with the same signature, perhaps for obvious reasons).

So two options are:

  1. Define private versions of ITensorMPS.contract and ITensorMPS.linsolve for the time being. We could call them something else, like ITensorMPS.alternating_update_contract and ITensorMPS.alternating_update_linsolve, or go back to the previous names ITensorMPS.itensortdvp_contract and ITensorMPS.itensortdvp_linsolve.
  2. Keep the current design but bump the version of ITensors.jl to v0.5.0 to mark this as a breaking change to protect users from using this version of ITensors.jl and the current version of ITensorTDVP.jl.

I would prefer to avoid 2. so in the end it may be best to just go back to the names itensortdvp_contract, itensortdvp_dmrg_x, itensortdvp_linsolve, and itensortdvp_tdvp. That may give us more flexibility with designing those respective functions in ITensorMPS.jl anyway.

@emstoudenmire
Copy link
Collaborator Author

I see, thanks for the detailed explanation. So then is the action item to close this PR? (Since it would keep the names as itensortdvp_tdvp, etc. ?)

@mtfishman
Copy link
Member

Yeah, looks like we can just close this and go with the previous design.

@mtfishman mtfishman closed this Apr 26, 2024
@mtfishman mtfishman deleted the itensortdvp_interface branch April 26, 2024 16:56
@emstoudenmire
Copy link
Collaborator Author

Ok, I'll update the other PR accordingly.

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