Conversation
|
I will finish this up tomorrow: I want to further test around and I need to finish up the docstrings. I will then also make the tests use the kwarg-based interface since that should get rid of a few lines. Perhaps we can get this merged tomorrow :-) |
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
I sadly won't have time to really look over this in detail today (and Monday is a holiday here), but I will look into it next week. Do you mind if I try out some changes and push them to your branch if I get to it?
|
wait, I'm in the middle of doing this, you just ruined my merge 😀 |
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
I think this is now okay to merge for me. There are definitely still a couple kinks left, especially with the Krylov dimension, but I think these really deserve their own PRs anyways.
Given that this is somewhat large/opnionated PR, let's get some more reviews in: @Yue-Zhengyuan and @leburgel , do you have any more comments? (Keeping in mind that we can still make changes in follow-up PRs)
|
Thanks for cleaning up the rest, looks great to me! I'd be fine with merging as is. Regarding the Krylov dimension: I think once we have #150 ready (hopefully soon), then we can hopefully use the TensorKit rrule per default also for the |
Yue-Zhengyuan
left a comment
Yue-Zhengyuan
left a comment
There was a problem hiding this comment.
Thanks to @pbrehmer for improving the user interface! I only have a few minor suggestions below about things more relevant to my use cases. In addition, we may also update the SU examples examples/heisenberg_evol/simpleupdate.jl and examples/hubbard_su.jl to use the new leading_boundary syntax (I can push the changes to this branch if you don't mind).
BTW, I think that the old way to run CTMRG by providing the parameters as a combined ctm_alg actually still works? Since the new syntax just helps users to combine them, so users don't have to do it by themselves.
|
Thanks for the comments. I would be very happy if you want to add the changes you are suggesting here |
| - `:fixed` : the differentiated CTMRG iteration uses a pre-computed SVD with a fixed set of gauges | ||
| - `:diffgauge` : the differentiated iteration consists of a CTMRG iteration and a subsequent gauge-fixing step such that the gauge-fixing procedure is differentiated as well | ||
| * `solver_alg::Union{KrylovKit.KrylovAlgorithm,NamedTuple}=(; alg=:$(Defaults.gradient_eigsolver)` : Eigen solver algorithm which, if supplied directly as a `KrylovKit.KrylovAlgorithm` overrides the above specified `tol`, `maxiter` and `verbosity`. Alternatively, it can be supplied via a `NamedTuple` where `alg` can be one of the following: | ||
| - `:arnoldi` : Arnoldi Krylov algorithm, see the [KrylovKit docs](https://jutho.github.io/KrylovKit.jl/stable/man/algorithms/#KrylovKit.Arnoldi) for details |
There was a problem hiding this comment.
More as a reminder for the future, we should enable external references when we update the documentation.
| χenv = 16 | ||
| svd_algs = [SVDAdjoint(; fwd_alg=TensorKit.SDD()), SVDAdjoint(; fwd_alg=IterSVD())] | ||
| projector_algs = [HalfInfiniteProjector] #, FullInfiniteProjector] | ||
| projector_algs = [:halfinfinite] #, :fullinfinite] |
There was a problem hiding this comment.
Kind of unrelated, but why is the full infinite projector commented out? What was the problem, and mainly, is there an issue tracking this?
There was a problem hiding this comment.
Good point: IIRC there were stability issues with the full-infinite scheme and the element-wise convergence. That was actually not the only thing where full-infinite did weird things. I'll take a quick look and file an issue!
Indeed, the old syntax still works and its intended as the "expert" mode since that exposes all the algorithm structs at once. So if you want to use that style you can still do so. |
|
Looks great overall, I really think this is a massive improvement. Left some final comments, I'm happy to actually apply the suggestions people agree with, just say so and tag me so there's no conflicts with who's working on what :) |
|
I can also quickly add all the remaining suggestions, thanks for the comments! :) |
Co-authored-by: Lander Burgelman <39218680+leburgel@users.noreply.github.com>
Co-authored-by: Lander Burgelman <39218680+leburgel@users.noreply.github.com>
Co-authored-by: Lander Burgelman <39218680+leburgel@users.noreply.github.com>
|
Alright, this is mergeable from my side, once the tests turn green! |
4 participants
In this PR we implement a keyword selector function which parses optimization kwargs onto the corresponding algorithm structs and outputs a
PEPSOptimizetofixedpoint. This simplifies the user interface since one doesn't have to create multiple nested algorithm structs.The same could be done also for
leading_boundarysince it is another frequently-used function with many algorithmic parameters.This idea follows KrylovKit's design which supplies a user-friendly kwarg-based method and an "expert" method. See e.g. the
eigselector.