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

Make more ITensorMPS internals visible #7

Merged
merged 1 commit into from
May 10, 2024
Merged

Conversation

emstoudenmire
Copy link
Contributor

This PR makes a few more ITensorMPS internal types and methods visible, in order for the ITensorQTT package tests to pass. They now pass so this should be all of these needed for that package.

@mtfishman
Copy link
Member

@emstoudenmire I would prefer rewriting whatever part of ITensorQTT.jl is using ITensors.ITensorMPS.makeL! and ITensors.ITensorMPS.makeR!, those seem so internal that they probably shouldn't be used in the first place.

@mtfishman
Copy link
Member

If that seems too involved I'm ok going ahead with this PR as it is, but we can use this as an opportunity to reassess what ITensorQTT.jl is doing.

@mtfishman
Copy link
Member

I see that it is being used in ITensorQTT to define a MPS linsolve function, maybe all of that code can be deleted since MPS-based linsolve is defined in ITensorTDVP/ITensorMPS. I think I was playing around with some variations of MPS linear solving but that was pretty experimental and can be removed.

@emstoudenmire
Copy link
Contributor Author

It's a good point. There are actually some other issues with that ITensorQTT linsolve code conflicting with ITensorTDVP.

I would not mind rewriting it along the lines you mentioned. But then deleting it is even easier and then we can close this PR.

So, delete it? (The ITensorQTT linsolve.jl file?)

@mtfishman
Copy link
Member

Perhaps this is good to do anyway since makeL! and makeR! are part of the interface of AbstractProjMPO so probably some advanced users are overloading those, so we should try to make the transition to ITensorMPS easier for those users.

@emstoudenmire
Copy link
Contributor Author

It's true – we might get some bug reports from a few users otherwise.

@mtfishman
Copy link
Member

Ok, I'll merge this as it is, and let's also keep ITensorQTT.jl as it is for now (besides the minimal updates needed to move to using ITensorMPS.jl) but have a longer term goal of simplifying it to not use ITensorMPS internals.

@mtfishman mtfishman merged commit 04faa15 into main May 10, 2024
9 checks passed
@mtfishman mtfishman deleted the itensormps_visibility branch May 10, 2024 23:50
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