Add adaptive tolerance scaling for boundary and gradient algorithms in variational optimization - #410
Add adaptive tolerance scaling for boundary and gradient algorithms in variational optimization#410leburgel wants to merge 9 commits into
Conversation
Codecov Report❌ Patch coverage is
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
pbrehmer
left a comment
There was a problem hiding this comment.
Up to one very minor thing, this looks good to me! I really like that this also clears up how the per-iteration information is logged.
Co-authored-by: Paul Brehmer <paul.brehmer@univie.ac.at>
| push!(gradnorms_unitcell, norm.(g.A)) | ||
| push!(times, (time_ns() - start_time) * 1.0e-9) | ||
| latest_gradnorms[] = norm.(unitcell(g)) | ||
| latest_time[] = (time_ns() - start_time) * 1.0e-9 |
There was a problem hiding this comment.
Is there a reason to ask for time_ns to then immediately convert to * 1e-9?
There was a problem hiding this comment.
No idea, I didn't really touch this. Nanosecond resolution for a PEPS optimization iteration doesn't sound like something that will be necessary anytime soon, so I can change it to time.
| produced by the `fg` call corresponding to the accepted step, into | ||
| `contraction_metrics`/`gradnorms_unitcell`/`times` | ||
| """ | ||
| function track_state_and_finalize!( |
There was a problem hiding this comment.
This is more of a suggestion about code organization, which would achieve the exact same thing, but I'm not that used to having many of these Ref values around as a way to keep things mutable, and combining this with an anonymous function factory might not be the most readable way of getting to this.
It might make sense to create a simple mutable struct for this, and then overload function calling? Something like
mutable struct TrackedFinalizer{A, B, C, D}
tol_state::A
latest_metrics::B
latest_gradnorms::C
latest_time::Float64 # I'm guessing?
const contraction_metrics::Vector{B}
const gradnorms_unitcell::Vector{C}
const times::Vector{Float64}
const finalize!
end
I'm not saying this is necessarily better, but I might have structured it more like this so just wanted to leave this suggestion to see what you think.
Additionally, it might even be possible to get rid of the mutable ref values and simply have a finalizer that resize!s the vector's by length + 1, and then just update the last element in the vector, which is of course mutable. (although that might lead to subtleties for the last iteration, maybe?)
There was a problem hiding this comment.
I actually like resize! a lot better, didn't think of that.
I'm fine with using a callable mutable struct instead, let me give this a go.
Adds adaptive tolerance scaling, where the tolerance of the contraction algorithm is set based on the current norm of the gradient (default scaling factor of
1.0e-3). The tolerance of the gradient algorithm in turn is set based on the contraction tolerance (default scaling factor1.0e1).On the README example, these default settings give a 1.5x speedup in walltime needed to reach a given energy value starting from the same initial guess. I'll see what the tests say for now, and do some more trials in the meantime.
At the same time, this reworks the way the per-iteration info metrics are stored and added. Previously, the
contraction_metricsandgradnorms_unitcellgot a new entry every time cost function was called. These function evaluations don't necessarily match the iterations of the optimization loop. In particular, entries were added for every rejected step during the line search, which just seemed needlessly confusing. I changed to workflow to record the current values in the cost function, but only add entries in thefinalize!routine, which is actually called after every iteration (i.e. every accepted step).This is also in anticipation of adding preconditioning, which naturally requires access to the current gradient norm.