Overhaul on error reporting of algorithms - #512
Conversation
…ns in time evolution
|
After the build completes, the updated documentation will be available here |
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Left some comments throughout, but as a more general idea here, since this is breaking anyways:
I think the entire idea of psi, envs, eps is probably not great to begin with, precisely because it is hard (or important) to keep that to meaning the same in every part. If we are making breaking changes anyways, it might be a convenient time to just go to a more KrylovKit-related approach where we just return an info struct, where we are then actually free to return the different quantities, and name them appropriately. This both keeps the signature the same everywhere, without unwantedly promising meaning to the number.
Thanks for taking the time to properly document many of these things by the way, this is definitely a welcome addition. There are some subtleties about the prose not lining up with the theory or the implementation, since especially for the convergence measures being practical had higher priority than being rigorous, and it seems like the language kind of mixes between the two. I'm not sure if you wanted to describe the theory or the implementation?
| ### Ground-state accuracy | ||
|
|
||
| [`find_groundstate`](@ref), [`leading_boundary`](@ref) and the iterative [`approximate`](@ref) algorithms return the quantity their `tol` is compared against. | ||
| For the sweeping algorithms ([`DMRG`](@ref), [`DMRG2`](@ref), [`VUMPS`](@ref), [`IDMRG`](@ref), [`IDMRG2`](@ref)) this is the Galerkin error: the norm of the local gradient projected orthogonally to the current state. |
There was a problem hiding this comment.
I don't think you can really project a gradient orthogonally to a state, so maybe easier is: "the norm of the gradient projected onto the local MPS manifold" or something similar?
There was a problem hiding this comment.
Yeah the wording was sloppy on my part, but wouldn't it also be more correct to say you're projecting onto the complement of the manifold? I can also just adapt calc_galerkins current docstring definition: "the overlap of the current state with the single-site derivative, projected onto the nullspace of the current state"
| [`find_groundstate`](@ref), [`leading_boundary`](@ref) and the iterative [`approximate`](@ref) algorithms return the quantity their `tol` is compared against. | ||
| For the sweeping algorithms ([`DMRG`](@ref), [`DMRG2`](@ref), [`VUMPS`](@ref), [`IDMRG`](@ref), [`IDMRG2`](@ref)) this is the Galerkin error: the norm of the local gradient projected orthogonally to the current state. | ||
| It vanishes exactly at a variational fixed point. | ||
| [`GradientGrassmann`](@ref) instead reports the norm of the Riemannian gradient from its optimizer. |
There was a problem hiding this comment.
I might be wrong here, but is that not the same gradient, up to second order things?
There was a problem hiding this comment.
So it's better to say at first order it's the same gradient, but at different metrics, and the tolerance still doesn't transfer because they're summed up differently?
| This is what a bond expansion (CBE) exists to reduce ([Li et al.](@cite li2024)). | ||
|
|
||
| * **Time-discretization error.** | ||
| The projector splitting is globally ``O(dt^2)`` for the symmetric back-and-forth sweep ([Lubich et al.](@cite lubich2015), [Paeckel et al.](@cite paeckel2019)), so it is controlled by `dt` alone. |
There was a problem hiding this comment.
I'm not sure I fully agree with the explanation/title combination here, I'd say the thing you are describing is the trotterization error, rather than a time-discretization error, although I agree this might be potato-potato 🙃
There was a problem hiding this comment.
At least within TDVP I think it's potato-potato, but maybe it's worthwhile generalising this to a "splitting error" since the time MPO methods split the Hamiltonian itself, while TDVP splits the tangent space projector.
|
Thanks for the review! I agree I was inconsistent in separating theory vs what's effectively done in the code. I think it's beneficial to give both, so I'll see how far I get in that. I'm also in favor of the info struct, so I'll try that out! |
borisdevos
changed the title
…o bd/tdvp-errors
|
Okay, a bunch has happened, and the goal of this PR has completely shifted, but the changes are better, and it's good that they're done at once (or at least shown here bunched up, there's an argument to splitting up some parts). I introduced I expanded on the docs a bunch more as well, correcting some of the false statements I made along the way. I think it reads more clearly now what a user could expect from these errors versus what they actually get. I think I'm less wrong than last time, but there might still be mistakes 🙃 |
| it means this particular error channel is absent. See the manual on [Errors and accuracy](@ref) | ||
| for what is *not* measured here. | ||
| """ | ||
| struct AlgorithmInfo{T <: Real} |
There was a problem hiding this comment.
Is there a benefit to using this struct vs say a NamedTuple? I'm a bit wary that not all algorithms have access to the same information, and therefore might not want to report the same information. I do like having some standardization with what fields/names mean what properties, but I am a bit scared about this not being very extensible.
For example, one other way to do this would be to have something like:
struct AlgorithmInfo
fields::Dict{Symbol, Any}
end(+ presumably some syntactic sugar to directly access fields etc)
This basically means that all algorithms are "type-stable" in the sense that they can return any information they want without altering the output type, but of course still needs a dynamic dispatch for actually accessing the fields itself, which I'm assuming here to never be performance-critical.
There was a problem hiding this comment.
I think what you're suggesting is to trade "same name, different meanings" with the opposite, right? The bad part with the former which I do here and tried to fix with elaborate docstrings is that e.g. normres does in fact mean something else depending on the algorithm. The opposite is having some return :galerkin and others :residual or something along those lines, which to me at least at the level of AlgorithmInfo makes it annoying to get to the information. I guess that's what you mean by sugar then accounting for this?
The struct as-is prints also nicely, which is to be considered, and it gives you the information immediately without having to call extra functions. All the fields are known à priori, the only thing is interpreting it, which again leads to the docstrings/docs being elaborate. It's indeed not extensible, but the current fields cover a lot of ground on what an algorithm would want to return, no?
There was a problem hiding this comment.
I think indeed having some return :galerkin and others :gradientnorm or anything like that, where names are tied to meanings, is exactly what I would expect. This still means that the relevant docstrings have to document what these mean, but I do think that gives us a lot more flexibility to actually return diagnostic data.
I guess I don't really agree with "the fields are known a priori" in the sense that you specifically have to check which ones are nothing, which is literally the same as having to check which fields are there. The only thing I'm suggesting here is to just make this fully dynamic from the beginning, only filling in "fields" that are present, which can easily be queried with keys, and additionally being trivially extensible (e.g. we could if we wanted to return iteration count, function application count, ... in a non-breaking way).
Having it print nicely is not really a great argument, it really isn't that hard to write the show method such that it prints the exact same information here as well 😉.
There was a problem hiding this comment.
If you put it that way, then I must agree with you 😄 but I see your point now, and this flexibility is definitely sturdier to future changes. I'll get to overhauling the overhaul
(My print argument was more when thinking about named tuples, because that would be type piracy to pretty print that, no?)
| ϵ_max::T | ||
| ϵ_total::T |
There was a problem hiding this comment.
In general I try to avoid unicode in propertynames, or basically anywhere where it is basically a forced choice, as there are still some parts where typing unicode is not the most convenient and having an ascii alternative is nice.
2 participants
[Relevant update deeper in conversation]
Description
The main motivation started with
TDVP2not reporting its truncation error. I noticed it could just make use ofgauge2!to remove duplication. While doing this, I realised multiple parts in the code didn't report their error, or didn't clarify clearly what the error actually means. In particular for the time evolution code which isn't variational, it made me realise thatϵcould mean anything. So this PR ended up expanding massively to also documenting per algorithm where relevant what the returned error represents.Details of the changes are mentioned in the changelog, and motivation for the errors in the docstrings or documentation. Importantly:
timestep/timestep!/time_evolve/time_evolve!now return(ψ, envs, ϵ).time_evolvealso logs more correctly. Tests added for this.Something I noticed along the way with
changebondsis that the meaning of its truncations differ too strongly to unify and justify returning the error. There's an argument to returning it for(VUMPS)SvdCutas the error there is genuinely a truncation error, but I didn't do that.Checklist
julia --project=test test/runtests.jl, or the relevant subset)docs/src/)[Unreleased]indocs/src/changelog.md, if this PR is user-facing (new feature, behavior change, bug fix, deprecation, or removal)