-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more convergence history to scfres #1049
base: master
Are you sure you want to change the base?
Add more convergence history to scfres #1049
Conversation
Hm, the API feels a bit ad hoc, and self_consistent_field already has lots of stuff in it. Can we somehow merge this with the callback API? Ie the callback would add stuff to |
A bit ugly, but something like this might also work? history_extra = []
callback = info -> push!(history_extra, info.<what needs to be saved>)`
self_consistent_field(...; callback) |
I added a specific callback that takes care of filling an history_extra dictionary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok to include the εF
if you need it @ClementineBarat, but about the ScfSaveHistory
struct I am not sure. What is the advantage of this struct over just writing a bunch of callbacks directly ?
Just to be clear: My problem is that you inevitably have to make some decisions when designing such a struct (e.g. you choose to call functions as fun(; info...)
) which work very well in the use case you have in mind, but which may not work well for others. Then others anyway have to write some bloat code to connect the SCF to their callback function.
Yes that make sense, no need to add my custom callback to DFTK. I removed all changes except for adding the history_εF but i can do without if it is just for me. |
I think it can be useful also for occupation functions, but @antoine-levitt may have an opinion here. @antoine-levitt If you are ok, I'd say we merge. |
src/scf/self_consistent_field.jl
Outdated
info_next = merge(info_next, (; energies, history_Etot, history_Δρ, history_εF, | ||
n_matvec)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the indention is weird, I'd do
info_next = merge(info_next, (; energies, history_Etot, history_Δρ, history_εF, | |
n_matvec)) | |
info_next = merge(info_next, (; energies, history_Etot, history_Δρ, history_εF, | |
n_matvec)) |
@@ -202,8 +206,7 @@ Overview of parameters: | |||
|
|||
info_init = (; ρin=ρ, ψ=ψ, occupation=nothing, eigenvalues=nothing, εF=nothing, | |||
n_iter=0, n_matvec=0, timedout=false, converged=false, | |||
history_Etot=T[], history_Δρ=T[]) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you're killing an empty line, which I think would help the structure of the code.
scfres already has quite a lot of stuff, maybe we can have a convergence_history field and move all the convergence logging stuff to there? |
Ok for me, sounds reasonable, although I'd just name it |
This PR adds history_εF to scfres and the possibility to save the convergence history of other data by passing a dictionary of functions that are evaluated and saved at each scf iteration.