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

throw error in case of order mismatch in ProjMPO-ITensor product #390

Merged
merged 3 commits into from
May 28, 2020

Conversation

orialb
Copy link
Contributor

@orialb orialb commented May 23, 2020

This PR adds a check to product(P::ProjMPO,wf::ITensor) which will throw an informative error if the order of P(wf) doesn't match wf, as discussed in #389.
I also implemented NDTensors.inds(P::ProjMPO).

The error message (for the case nsite(P)==2) would look like this:
image

I should probably add some tests, but there was no projmpo.jl test file, should I create a new one or should the tests be in dmrg.jl test file?

Also there are some multi-line function calls there and I wasn't fully sure what formatting guidelines should be followed, so let me know if formatting is off (maybe worth adding some contributors guide section somewhere).

@mtfishman
Copy link
Member

mtfishman commented May 26, 2020

Hey Ori,

Looks like a good check to add.

We could also add a compile-time check, the advantage would be that it would make this function more type-stable (the order of the output should be inferable). I think we could just make the function signature:

function product(P::ProjMPO,
                 v::ITensor{N})::ITensor{N} where {N}
   [...]
end

The runtime check you added should get called as well and is useful to have, so I think we should have both this extra type information and the check and error message you added.

Having an inds function for ProjMPO is an interesting idea. The main reason I'm kind of hesitant about it is that I think we would want to make the code for all of the "projected operator linear maps" (ProjMPO, ProjMPO_MPS, ProjMPOSum) as simple and generic as possible, so it is easy for other people to implement their own. People might get confused about why inds(::ProjMPO) is there and think they have to implement it as well, which is not so trivial in general. Couldn't we just print inds(v) and inds(Hv) in the error message?

Also it is a good idea to have guidelines for contributing to ITensors.jl. I've started an "advanced usage" section of the docs here: #387, I can start adding that there. I think with multi-line calls you are referring to here, it may be better to split it into multiple lines with temporary variables.

-Matt

@mtfishman mtfishman mentioned this pull request May 26, 2020
12 tasks
@orialb
Copy link
Contributor Author

orialb commented May 27, 2020

Having an inds function for ProjMPO is an interesting idea. The main reason I'm kind of hesitant about it is that I think we would want to make the code for all of the "projected operator linear maps" (ProjMPO, ProjMPO_MPS, ProjMPOSum) as simple and generic as possible, so it is easy for other people to implement their own. People might get confused about why inds(::ProjMPO) is there and think they have to implement it as well, which is not so trivial in general. Couldn't we just print inds(v) and inds(Hv) in the error message?

We could print just inds(Hv), I just thought having the indices of the ProjMPO would be slightly more useful for debugging (because it tells the user exactly what kind of tensor it expects).
How about giving that function another name, or clarifying in the documentation (could be in a docstring for inds(ProjMPO)) that the only thing a ProjMPO like object needs to implement is product ?
But if it's too much of a hassle then I can remove it.

I'll add the compile time check.

@mtfishman
Copy link
Member

I think we should remove it for now, since inds(v) and inds(Hv) should give enough information.

@orialb
Copy link
Contributor Author

orialb commented May 27, 2020

updated the PR with the type annotation and removed inds(::ProjMPO).

@mtfishman
Copy link
Member

Looks great, thanks!

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