Skip to content

Contract mpo mpo #45

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.

Sign up for GitHub

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

Closed
shinaoka wants to merge 2 commits into from
Closed

Contract mpo mpo #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff7c1dd
Implement MPO-MPO contraction
shinaoka Jan 11, 2023
141c2c8
Refactor test_contract_mpo.jl
shinaoka Jan 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/treetensornetworks/solvers/contract_mpo_mps.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,20 @@ function ITensors.contract(
init_mps = deepcopy(init_mps)
init_mps = sim(linkinds, init_mps)
Ai = siteinds(A)
ti = Vector{Index}(undef, n)
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
Comment on lines +33 to 46

@mtfishman mtfishman Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explain what this is doing?

@shinaoka shinaoka Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

@mtfishman mtfishman Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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).

@mtfishman mtfishman Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Along those lines, in general I would avoid doing index replacements/forcing indices to match, if that is what this code is doing.

@shinaoka shinaoka Jan 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

@mtfishman mtfishman Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@mtfishman mtfishman Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@mtfishman mtfishman Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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!

replace_siteinds!(init_mps, ti)

t = Inf
reverse_step = false
Expand Down
25 changes: 25 additions & 0 deletions test/test_treetensornetworks/test_solvers/test_contract_mpo.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,29 @@ using Test
@test inner(psit, Hpsi) ≈ inner(psit, H, psi) atol = 1E-4
end

function asMPS(M::MPO, sites)
M_ = MPS(length(sites))
for n in eachindex(sites)
M_[n] = M[n]
end
return M_
end
Comment on lines +54 to +60

@mtfishman mtfishman Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would remove this and just use MPS([A for A in M]) directly in the test.


@testset "Contract MPO-MPO" begin
nbit = 5
sites = siteinds("Qubit", nbit)
M1 = randomMPO(sites) + randomMPO(sites)
M2 = randomMPO(sites) + randomMPO(sites)

# The function `apply` does not work correctly with the mapping-MPO-to-MPS trick.
M1 = replaceprime(M1, 1=>2, 0=>1)

M2_ = asMPS(M2, sites)

M12_ref = asMPS(contract(M1, M2; alg="naive"), sites)
M12 = contract(M1, M2_; alg="fit")

@mtfishman mtfishman Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this only testing MPO-MPS contraction?

@shinaoka shinaoka Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, I got your point. We are testing MPO-(MPS with two site indices) here. I'll rename it correctly.

@mtfishman mtfishman Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@shinaoka shinaoka Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Originally, I intended to support MPO-MPO contraction. But, the latter case should work. I will add a test on the latter case.

@mtfishman mtfishman Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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).


@test M12_ref ≈ M12
end

nothing