Contract mpo mpo - #45
Conversation
| init_mpsi = siteinds(init_mps) | ||
| for j in 1:n | ||
| ti = nothing | ||
| for i in Ai[j] | ||
| if !hasind(psi0[j], i) | ||
| ti[j] = i | ||
| ti = i | ||
| break | ||
| end | ||
| end | ||
| if ti !== nothing | ||
| ci = commoninds(init_mpsi[j], A[j])[1] | ||
| replaceind!(init_mps[j], ci=>ti) | ||
| end | ||
| end |
There was a problem hiding this comment.
Could you explain what this is doing?
There was a problem hiding this comment.
This ensures that phi0 and init_mps have the same site indices. This originates from the old code, but I feel this is hard to understand. Why not throwing an error if the two MPSs do not have the site indices?
There was a problem hiding this comment.
In general, I prefer to write the code where it is as flexible as possible about the indices, so it should be ok if there are missing site indices (for example I have hit examples like that in ITensorQTT.jl).
There was a problem hiding this comment.
Along those lines, in general I would avoid doing index replacements/forcing indices to match, if that is what this code is doing.
There was a problem hiding this comment.
Oh, there was a typo. This code ensures that A phi0 and init_mps have the same site indices.
What example do you have in ITensorQTT.jl? A acts only on a subset of site indices (subset of tensors) of phi0?
There was a problem hiding this comment.
I would vote for just not checking or fixing anything at first to allow the most flexibility, and if issues come up from that we can add checks for particular issues.
There was a problem hiding this comment.
For context, I think in general in the past we have over-specialized code too much in functions like this, and the goal of ITensorNetworks.jl is to be as agnostic as possible about particular tensor network structures.
There was a problem hiding this comment.
Along those lines, as of #44 the underlying generic tdvp function now works for tree tensor networks, so with some minor modifications it should be easy to make this function work for that case as well!
| function asMPS(M::MPO, sites) | ||
| M_ = MPS(length(sites)) | ||
| for n in eachindex(sites) | ||
| M_[n] = M[n] | ||
| end | ||
| return M_ | ||
| end |
There was a problem hiding this comment.
I would remove this and just use MPS([A for A in M]) directly in the test.
| M2_ = asMPS(M2, sites) | ||
|
|
||
| M12_ref = asMPS(contract(M1, M2; alg="naive"), sites) | ||
| M12 = contract(M1, M2_; alg="fit") |
There was a problem hiding this comment.
Isn't this only testing MPO-MPS contraction?
There was a problem hiding this comment.
Ah, I got your point. We are testing MPO-(MPS with two site indices) here. I'll rename it correctly.
There was a problem hiding this comment.
I guess I'm unclear about what the new functionality is. Is it MPO-MPO contraction, or a more general MPO-MPS contraction where the MPS can have dangling (uncontracted) indices?
There was a problem hiding this comment.
Originally, I intended to support MPO-MPO contraction. But, the latter case should work. I will add a test on the latter case.
There was a problem hiding this comment.
One goal of ITensorNetworks.jl is to make code "just work" whether it involves MPO or MPS, and in fact we may just not have separate types (that is still being debated). But it would be best if this kind of code didn't care much if the inputs are MPO or MPS (and as of #44, TTNO/TTNS types as well).
|
|
OK, once #44 has been merged, which I was not aware of, this PR does not make sense. I will close it and work on the current main branch. |
|
This has been addressed in #46 . |
2 participants
Moved from ITensor/ITensorTDVP.jl#48
We may need to polish the interface a bit.
Does it make sense to introduce a specialization of
apply(MPO, MPO, ...)?