Add C4v CTMRG #321
Add C4v CTMRG #321
Conversation
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
|
This PR already involves lots of things. For QR CTMRG, besides getting projectors from QR instead of SVD, the way to enlarge corners is also different. Maybe we focus on the |
Completely agree, I anyway assumed this would go into a separate PR. Also I apologize for making this PR so big, I found it somewhat hard to split up the changes needed to make the whole apparatus compatible with C4v CTMRG. |
One approach is to make |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Co-authored-by: Yue Zhengyuan <yuezy1997@icloud.com>
I have thought about this but this would be rather annoying as an intermediate stage I think since it would break a lot of the general CTMRG functions that I use now leading to a lot of code duplication and extra work (that I don't really have time for at the moment). I'd rather do it once but properly. I hope that's fine! |
|
I think in principle the tests should run through now. Once we have merged #264 and some details have been settled, I will add a small example to the docs on how to use C4v CTMRG. |
|
Since we are going to also introduce CTMRG on triangular lattices in #324 (which also has a C6v specialization), it may be better to reorganize files into sub-folders under
Similarly, the pure contraction functions in |
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Overall I don't think I have too many comments on this.
I definitely share your view that it is a rather large PR and that it sucks a bit to take on this much code debt, but I also don't actually have any suggestions as to how to avoid this without having to really start rewriting everything so I guess we just have to deal with this.
I would maybe try and clean up a little more, I don't think the QR stuff is working already right? Can we just not deal with that just yet, and instead try to merge this first?
Otherwise I left a couple small nitpicking naming questions and comments, but I think overall this looks like great work, thanks!
| function physical_flip(A::AbstractTensorMap{T, S, N, 1}) where {T, S, N} | ||
| return flip(A, 2:N) | ||
| end | ||
| function project_hermitian(E::AbstractTensorMap{T, S, N, 1}) where {T, S, N} |
There was a problem hiding this comment.
Can we use a different name for this? I think MatrixAlgebraKit already exports this as well.
| - `:truncspace` : Additionally supply truncation space `η`; truncate according to the supplied vector space | ||
| - `:trunctol` : Additionally supply singular value cutoff `η`; truncate such that every retained singular value is larger than `η` | ||
| * `svd_alg::Union{<:SVDAdjoint,NamedTuple}` : SVD algorithm for computing projectors. See also [`SVDAdjoint`](@ref). By default, a reverse-rule tolerance of `tol=1e1tol` where the `krylovdim` is adapted to the `env₀` environment dimension. | ||
| * `decomposition_alg::Union{<:DecompositionAdjoint,NamedTuple}` : Tensor decomposition algorithm for computing projectors. See e.g. [`SVDAdjoint`](@ref). |
There was a problem hiding this comment.
| * `decomposition_alg::Union{<:DecompositionAdjoint,NamedTuple}` : Tensor decomposition algorithm for computing projectors. See e.g. [`SVDAdjoint`](@ref). | |
| * `decomposition_alg` : Tensor decomposition algorithm for computing projectors. See e.g. [`SVDAdjoint`](@ref). |
I wonder if it really makes sense to have that type description in the docstring. I don't think most people would know what that type alias means?
| function calc_elementwise_convergence( | ||
| envfinal::CTMRGEnv{C}, envfix::CTMRGEnv{C′}; atol::Real = 1.0e-6 | ||
| ) where {C, C′} | ||
| Cfinal = (C <: DiagonalTensorMap) ? convert.(TensorMap, envfinal.corners) : envfinal.corners |
There was a problem hiding this comment.
QuantumKitHub/TensorKit.jl#361
please open issues for things like this ❤️
| using KrylovKit: Lanczos, BlockLanczos | ||
| const KrylovKitCRCExt = Base.get_extension(KrylovKit, :KrylovKitChainRulesCoreExt) | ||
|
|
||
| """ |
There was a problem hiding this comment.
Can we merge all the MatrixAlgebraKit pullbacks in one struct?
Would be nice to avoid some of this duplicated boilerplate/docstrings etc
| end | ||
|
|
||
| # hacky way of computing the truncation error for current version of eigh_trunc! | ||
| # TODO: replace once TensorKit updates to new MatrixAlgebraKit which returns truncation error as well |
There was a problem hiding this comment.
Then it may be better to address #314 first, which still requires changes in MPSKit to be released.
| function isfulleig(alg::FixedEig) | ||
| if isnothing(alg.D_full) || isnothing(alg.U_full) | ||
| return false | ||
| else | ||
| return true | ||
| end | ||
| end |
There was a problem hiding this comment.
| function isfulleig(alg::FixedEig) | |
| if isnothing(alg.D_full) || isnothing(alg.U_full) | |
| return false | |
| else | |
| return true | |
| end | |
| end | |
| isfulleig(alg::FixedEig) = !isnothing(alg.D_full) && !isnothing(alg.U_full) |
There was a problem hiding this comment.
Is there a specific reason to use U instead of V for the eigenvectors? It's slightly odd to me to change that convention here instead of just following the MatrixAlgebraKit one?
There was a problem hiding this comment.
Can we remove this file for now?
There was a problem hiding this comment.
@pbrehmer I'm also OK with removing QR stuff now instead of just commenting it out.
| using MatrixAlgebraKit: TruncationStrategy, NoTruncation, LAPACK_EighAlgorithm, truncate | ||
| using MatrixAlgebraKit: eigh_pullback!, eigh_trunc_pullback!, findtruncated, diagview | ||
| using TensorKit: AdjointTensorMap, SectorDict, Factorizations.TruncationSpace, | ||
| throw_invalid_innerproduct, similarstoragetype | ||
| using TensorKit.Factorizations: _notrunc_ind | ||
| using KrylovKit: Lanczos, BlockLanczos |
There was a problem hiding this comment.
Can we import modules only in src/PEPSKit.jl?
(BTW, there is a using MPSKit: InfiniteEnvironments in src/environments/vumps_environments.jl that I suggest to also be moved to src/PEPSKit.jl.)
| E_ref = -0.6602310934799577 | ||
|
|
||
| # Heisenberg model assuming C4v symmetric PEPS and environment, which only evaluates necessary term | ||
| function heisenberg_XYZ_c4v(lattice::InfiniteSquare; kwargs...) |
There was a problem hiding this comment.
How about adding this directly to src/operators/models.jl so it can be accessed by users? (also asking for @lkdvos's opinion)
In this PR we will finally add a dedicated C4v CTMRG algorithm. I started with the standard version using
eighprojectors but we can easily also implement a QR version by making aC4vQRProjectoralgorithm and the correspondingc4v_projectormethod. (@Yue-Zhengyuan said he would be up for doing this :-)) This C4v routine is completely differentiable using naive AD,:diffgaugeand:fixedmode.Note that I needed to generalize the
gauge_fixmethod forCTMRGEnvs to not have separate methods for all the C4v stuff. Same thing is true for environment initialization, where it would be nice to have aninitialize_environmentmethod where one can dispatch on an algorithm. Here we'll have to wait until we merge #264.Let me also say that it would eventually be nicer to have a dedicated
C4vCTMRGEnvsince this would make things cleaner. However, this would entail a larger rewrite of quite a few things, so I propose that I postpone this rewriting and cleaning up process until I have more resources for that again. (In general I feel like we should clean up and refactor some stuff again since the codebase has been growing quite a bit.) Anyways, for now it would be nice to have a working C4v CTMRG that people can use.This is still very much under construction but I'll finish up things and adjust the test suite tomorrow.
Closes #258.