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] Bond dimensions produced by randomCircuitMPS #870

Closed
wants to merge 9 commits into from

Conversation

markusschmitt
Copy link

Description

Fixes ITensor/ITensorMPS.jl#69

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

  • Test bond dimensions for L=10. This test is included in test/mps.jl as testset "randomCircuitMPS bond dimensions".
using ITensors
using Test

L=10
chi=128

sites = siteinds("S=1/2", L)
mps=myRandomCircuitMPS(ComplexF64,sites,chi)

expected_dims = [2, 4, 8, 16, 32, 16, 8, 4, 2]
for i in 1:9
  l = getfirst(x->hastags(x,"Link,l=$(i)"),inds(mps[i]))
  @test dim(l) == expected_dims[i]
end

Checklist:

  • My code follows the style guidelines of this project. Please run using JuliaFormatter; format(".") in the base directory of the repository (~/.julia/dev/ITensors) to format your code according to our style guidelines.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.

Markus added 3 commits March 17, 2022 13:34
@@ -144,15 +144,28 @@ function randomCircuitMPS(

l = Vector{Index}(undef, N)

# Compute the bond dimension for each link.
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have this as a separate function, so we could use it other places in the code and simplify the code here.

Maybe linkdims_upper_bound? It doesn't have to be exported, just internal use for now.

@mtfishman
Copy link
Member

Thanks @markusschmitt.

My major concern here would be ensuring that by truncating in this way the orthogonality center is still at site 1. Would you mind checking that?

@markusschmitt
Copy link
Author

Yes, let me check that. I am actually not sure.

@mtfishman
Copy link
Member

If it does turn out to be an issue, a simple fix would be to determine at what site the orthogonality gets broken (say site n), set the orthogonality center range as 1:n, and then call orthogonalize!(psi, 1), so that it does the minimal orthogonalization needed. But hopefully there won't be anything to fix.

@mtfishman
Copy link
Member

@dylex do you know why the Jenkins test failed here? I've been seeing occasional errors, can we just increase the timeout time?

@dylex
Copy link
Collaborator

dylex commented Mar 17, 2022

I thought I fixed this but maybe it's still not right. I'll take a closer look. In the mean time I manually re-ran the test and now it's failing for non-timeout reasons at least.

src/mps/mps.jl Outdated Show resolved Hide resolved
@mtfishman
Copy link
Member

I thought I fixed this but maybe it's still not right. I'll take a closer look. In the mean time I manually re-ran the test and now it's failing for non-timeout reasons at least.

Thanks. Indeed, this PR broke a test. Is there a way that I can rerun tests (without making a new commit)? I didn't see a way from the Jenkins online interface (https://jenkins.flatironinstitute.org/blue/organizations/jenkins/ITensors.jl/detail/PR-870/2/pipeline).

@dylex
Copy link
Collaborator

dylex commented Mar 17, 2022

If you register a user on jenkins I can add you to be able to trigger builds.

@mtfishman
Copy link
Member

If you register a user on jenkins I can add you to be able to trigger builds.

That would be great, thanks. I just registered with the User ID "Matthew" and name "Matthew Fishman".

@emstoudenmire
Copy link
Collaborator

Nice to see this "bounding" approach to solving the issue, Markus. I think if we can verify even theoretically that the extra truncations still preserve or guaranteed the right-orthogonality property of the MPS tensors (so that the orthogonality center remains well-defined and at site 1) then we're in business.

@emstoudenmire
Copy link
Collaborator

I think the right-orthogonality may be guaranteed to remain true for any value of chi, including the new values being determined by the code in this PR. The reason is that chi is fed into the function _rmatrix, which makes a unitary (or orthogonal if real) matrix of the requested dimensions. So as long as the column dimensions of these matrices are greater than or equal to the row dimensions (in this case, as long as chi <= dim(sites[j]) * dim(l[j])) then the resulting tensors will be right-orthogonal. We could add an assertion that checks the above inequality always holds to ensure orthogonality.

@markusschmitt
Copy link
Author

@emstoudenmire: I think the condition chi <= dim(sites[j]) * dim(l[j]) is already ensured in the current version when calculating the maximal bond dimensions.

I also checked the orthogonality as follows:

using Test
using ITensors

L=6
chi=128

sites = siteinds("S=1/2", L)

mps=randomCircuitMPS(ComplexF64,sites,chi)

right_contracted = mps[L] * conj(prime(mps[L], tags="Link"))
@test isapprox(norm(right_contracted-delta(inds(right_contracted))), 0.0, atol=1e-14)
for j in (L - 1):-1:2
    right_contracted = (mps[j] * right_contracted) * conj(prime(mps[j], tags="Link"))
    @test isapprox(norm(right_contracted-delta(inds(right_contracted))), 0.0, atol=1e-14)
end

This could be added as a test, possibly with different smaller chi to check for the cases mentioned by Miles.

@emstoudenmire
Copy link
Collaborator

Great – I just checked and we do already have a similar test in our test/mps.jl file, so no need to add another one, but it's good you did that check yourself. The new test you added is good to have to ensure the bond dimensions now have the minimal values.

The current test isn't passing, though, because randomCircuitMPS is not exported by the ITensors module, as it's an internal function only. Please update the test to use the randomMPS interface instead so that the tests can pass.

Also it looks like you'll need to make the update pointed out by Matt, about changing the name myRandomCircuitMPS back to randomCircuitMPS in src/mps/mps.jl.

test/mps.jl Outdated Show resolved Hide resolved
@markusschmitt
Copy link
Author

Sorry, I am only getting back to this now. With the new commits the tests should pass. I had to slightly change another test as well.

@codecov-commenter
Copy link

Codecov Report

Merging #870 (ec8c9a8) into main (6a83a83) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head ec8c9a8 differs from pull request most recent head 238e4dc. Consider uploading reports for the commit 238e4dc to get more accurate results

@@            Coverage Diff             @@
##             main     ITensor/ITensors.jl#870      +/-   ##
==========================================
+ Coverage   67.12%   67.14%   +0.02%     
==========================================
  Files          88       88              
  Lines        9389     9395       +6     
==========================================
+ Hits         6302     6308       +6     
  Misses       3087     3087              
Impacted Files Coverage Δ
src/mps/mps.jl 91.91% <100.00%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a83a83...238e4dc. Read the comment docs.

@mtfishman mtfishman added the mps Issues related to MPS/MPO functionality label May 6, 2024
@mtfishman mtfishman changed the title Bond dimensions produced by randomCircuitMPS [ITensorMPS] Bond dimensions produced by randomCircuitMPS May 6, 2024
@mtfishman mtfishman added the ITensorMPS Issues related to the ITensorMPS submodule label May 6, 2024
@mtfishman
Copy link
Member

This issue should get transferred to ITensorMPS.jl, I'll close it since the corresponding issue is being tracked there: ITensor/ITensorMPS.jl#69.

@mtfishman mtfishman closed this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ITensorMPS Issues related to the ITensorMPS submodule mps Issues related to MPS/MPO functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ITensors] [BUG] randomMPS doesn't return MPS with minimal bond dimension on the boundaries
5 participants