-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add simple update algorithm #97
Conversation
Pb improve sequential
Pb improve sequential 2
Co-authored-by: Lukas Devos <[email protected]>
Co-authored-by: Lukas Devos <[email protected]>
I found that my simple update test takes abnormally long time to run on Ubuntu (181m58.6s) and Windows (44m09.8s), but seems fine on macOS (14m49.1s). Seems that there are still lots of places for improvement... |
Using MPSKitModels, the functions gen_siteop and gen_bondop are not necessary anymore. The relevant tensors and their tensor product can be defined directly from MPSKitModels
Actually, the multithreading in the CI seems to have gotten disabled here: 3a9eb40 (the default is just 1 thread on CI) I fixed the complaints about the required checks, this was a naming thing. |
Whoops, okay I wasn't aware that multi-threading was disabled all along. But I agree, that's a separate issue we should fix in a separate PR. Other than that the PR looks pretty good, it's quite a big step forward! I will quickly add the function signatures in the docstrings. |
I added the function signatures as well as some cosmetic changes, and I merged the Heisenberg tests. This is good to go for me once the tests turn green. Big big thanks to @Yue-Zhengyuan for the great effort! |
I checked the stacktraces of the failing tests, and I think the issue is a combination of some tolerances and a bug in KrylovKit: The eigensolver in the rrule is failing to converge all requested values, and as a result there is an out of bounds error due to the aforementioned bug in KrylovKit. I think we should be able to avoid running into this altogether, by ensuring that the tolerances are set such that everything converges? |
Thanks @pbrehmer for updating the docstring! I just found that there may be some mistakes on my part, and will fix them soon. By the way, I'm not quite familiar with defining the |
When fixing my definition of Base.eltype(env::CTMRGEnv) = eltype(env.corners[1]) However for PEPS/PEPO, it's defined as the type of the tensor Base.eltype(T::InfinitePEPO) = eltype(T.A)
Base.eltype(T::InfinitePEPS) = eltype(T.A) For example, using TensorKit
using PEPSKit
envs = CTMRGEnv(2, 2, 2; unitcell=(1, 2))
println(eltype(envs))
peps = InfinitePEPS(2, 2, 2; unitcell=(1, 2))
println(eltype(peps)) Output:
Should we change the definition for Base.eltype(T::InfinitePEPO) = eltype(T.A[1])
Base.eltype(T::InfinitePEPS) = eltype(T.A[1]) |
It's almost correct - yesterday I missed a tiny error. When deriving the adjoint for complex variables, one also needs to take the complex conjugate corresponding to
Yes, you're right. However, it's not a big issue since the more relevant way to access the scalar element type is using Nonetheless, it makes sense to define |
@pbrehmer Thanks for catching the rrule error! Could you also help add the definition of By some test I discover that changing the |
Well, my feeling is that Regarding |
OK, it is then reasonable we don't add However, the new definition of |
Ahh thanks for catching that, I didn't realize Regarding the |
Actually, the fix for eltype should be the other way around: that function is typically reserved for the type of the result of indexing or iterating, which is why it affects similar. At some point we realized that we need to also have access to the number type, so we introduced scalartype, but apparently the CTMRGEnv didn't get updated. In principle, I'd say it isn't even very well-defined for that, since it could be either an edge or a corner, but for the peps and pepo it really is |
In that case I would suggest just scrapping |
So the Heisenberg test does optimize if the derivative of the SVD is performed using This circumvents Nonetheless, I'd be fine with merging the PR like this - I think we can tackle the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to for me too!
This PR adds the core part of the simple update (arXiv 0806.3719) algorithm.
Main changes:
InfiniteWeightPEPS
, which is the collection of tensors on lattice vertices and bond weights. The weights are diagonal Schmidt matrices on the bonds of the iPEPS, and serve as a crude approximation of the environment of each site tensor. They are wrapped in the new structSUWeight
.ctmrg_leftmove
fromctmrg_iter
, which only perform the left move for one column (of the sequential CTMRG (Make:sequential
act column-wise #90)). It will be used in the full update (to be added later).LocalOperator
s.src/operators/models.jl
.test/heisenberg.jl
, and an example (by @Sander-De-Meyer) on Hubbard model at half-filling atexamples/hubbard_su.jl
.Limitations:
Things to do before merging:
Add more constructors for(postponed, when there is absolute need).InfiniteWeightPEPS
LocalOperator
format.