Change internal fixed-point API to (x, info) = f(x, info) to simplify SCF metadata tracking #811
Conversation
|
Since there are conflicts against master the CI will not run. @epolack @niklasschmitz You must first resolve the conflicts and/or rebase. |
|
@mfherbst, thanks. |
If you want to continue the development I'd say that's the simplest. You can already get an idea what's the issue right now from https://git.uni-paderborn.de/herbstm/DFTK.jl/-/jobs/123457 by the way. |
|
Reopening now after pulling, let's see what the CI says |
niklasschmitz
changed the title
| # Tolerance and maxiter are only dummy here: Convergence is flagged by is_converged | ||
| # inside the fixpoint_map. | ||
| solver(fixpoint_map, ρout, maxiter; tol=eps(T)) | ||
|
|
||
| _, info = solver(fixpoint_map, ρ, info_init, maxiter; tol=eps(T)) # TODO ?? why dummy? |
There was a problem hiding this comment.
On that TODO: I think I understand now the comment above for custom convergence flagging - I wonder whether we could move this to the fixed-point solvers API though? instead of specifying the tolerance, one could do something like solver(fixpoint_map, ρ, info_init, maxiter; is_converged_fn=is_converged_fn)?
There was a problem hiding this comment.
Agree 100%. The is_converged should be executed in the solver. The thing with the converged flag has always been a hack to make NLSolve happy.
src/scf/scf_solvers.jl
Outdated
| @@ -57,7 +60,7 @@ CROP-accelerated root-finding iteration for `f`, starting from `x0` and keeping | |||
| a history of `m` steps. Optionally `warming` specifies the number of non-accelerated | |||
| steps to perform for warming up the history. | |||
| """ | |||
| function CROP(f, x0, m::Int, max_iter::Int, tol::Real, warming=0) | |||
| function CROP(f, x0, info0, m::Int, max_iter::Int, tol::Real, warming=0) | |||
There was a problem hiding this comment.
@antoine-levitt Shall we just kill CROP ? It was an experiment anyway, which is not used or tested very much at all.
There was a problem hiding this comment.
It's useful to have since it parallels the anderson implementation, and we might want to look at it again in 5 years. Why can't info be an optional arg (or kwarg) so that solvers don't have to bother with it?
There was a problem hiding this comment.
If we want the solvers to be able to use the isconverged callback it somehow needs an info. But perhaps we can have a default-scfres function, which would be the default for the arg.
| # Tolerance and maxiter are only dummy here: Convergence is flagged by is_converged | ||
| # inside the fixpoint_map. | ||
| solver(fixpoint_map, ρout, maxiter; tol=eps(T)) | ||
|
|
||
| _, info = solver(fixpoint_map, ρ, info_init, maxiter; tol=eps(T)) # TODO ?? why dummy? |
There was a problem hiding this comment.
Agree 100%. The is_converged should be executed in the solver. The thing with the converged flag has always been a hack to make NLSolve happy.
src/scf/scf_solvers.jl
Outdated
| @@ -11,12 +11,13 @@ Create a damped SCF solver updating the density as | |||
| `x = β * x_new + (1 - β) * x` | |||
| """ | |||
| function scf_damping_solver(β=0.2) | |||
| function fp_solver(f, x0, max_iter; tol=1e-6) | |||
| function fp_solver(f, x0, info0, max_iter; tol=1e-6) | |||
There was a problem hiding this comment.
While we are at this: Drop the tol flag from here (taken care of by the new is_converged kwarg and also make maxiter a kwarg (has always been bothering me that it's not).
src/scf/scf_solvers.jl
Outdated
| function scf_CROP_solver(m=10) | ||
| function crop_solver(f, x0, info0, max_iter; tol=1e-6) | ||
| function residual_f(x, info) |
There was a problem hiding this comment.
Puh ... that's not easy to process. I really feel like removing the CROP stuff.
PR from @niklasschmitz. Will fix the tests when CI completes.