Skip to content
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

First go at flexible SCF convergence criteria #126

Merged
merged 3 commits into from Feb 14, 2020
Merged

Conversation

@mfherbst
Copy link
Member

mfherbst commented Feb 13, 2020

I'll do a cleanup pass if you agree with the general design @antoine-levitt. Allows us to play easily with SCF convergence criteria.

@antoine-levitt

This comment has been minimized.

Copy link
Member

antoine-levitt commented Feb 13, 2020

Why not directly callback functions? They get passes info, they return true or false if they're converged or not

@antoine-levitt

This comment has been minimized.

Copy link
Member

antoine-levitt commented Feb 13, 2020

If it's to separate printing and termination, we can just have a list of callbacks. We terminate when all (or maybe any?) of the callbacks are finished.

@mfherbst

This comment has been minimized.

Copy link
Member Author

mfherbst commented Feb 13, 2020

I thought about using the callback, but then I figured that the two things are really kind of orthogonal. I guess what one could do is have a list of functions and let each of them update the info (take it, do what you want and return it). If the "converged" flag is set to true by one of them, that means that the iterations are to be stopped. But then you can also quickly add other things to the SCF if you wanted.

Why I did not do that was that if you only want to change the convergence, than self_consistent_field(convergence=EnergyDifference(0.5)) is super clear, whereas self_consistent_field(callback=[default_print, convergence_energy_difference(0.5)]) is less clear to me. But one could argue it's low-level.

@mfherbst mfherbst force-pushed the flexible-convergence branch from 8364215 to cf31ae9 Feb 13, 2020
@antoine-levitt

This comment has been minimized.

Copy link
Member

antoine-levitt commented Feb 13, 2020

I agree we should have separate callbacks for printing and for stopping. What I really don't like is the mutation (mutation baaaad). How about we just have a stopping criterion that has the form of a callback? Then we can build more sophisticated stuff on top later on if we want to

@mfherbst mfherbst force-pushed the flexible-convergence branch from 7634324 to 1c910d4 Feb 13, 2020
@mfherbst

This comment has been minimized.

Copy link
Member Author

mfherbst commented Feb 13, 2020

@antoine-levitt Feel free to merge if you agree with the current design.

src/scf/scf_solvers.jl Outdated Show resolved Hide resolved
src/scf/scf_solvers.jl Outdated Show resolved Hide resolved
src/scf/self_consistent_field.jl Show resolved Hide resolved
src/scf/self_consistent_field.jl Outdated Show resolved Hide resolved
@mfherbst mfherbst force-pushed the flexible-convergence branch from a47c755 to 8d9c96a Feb 13, 2020
@mfherbst mfherbst force-pushed the flexible-convergence branch from 8d9c96a to e0fb004 Feb 13, 2020
@mfherbst mfherbst merged commit 0826daf into master Feb 14, 2020
3 checks passed
3 checks passed
Run DFTK examples
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 79.394%
Details
@mfherbst mfherbst deleted the flexible-convergence branch Feb 14, 2020
@antoine-levitt

This comment has been minimized.

Copy link
Member

antoine-levitt commented Feb 14, 2020

You mean the Float32? It just means that the specified tolerance for the stopping criterion was too tight. I think we want to keep that - if only for testing how far we can push it

@mfherbst

This comment has been minimized.

Copy link
Member Author

mfherbst commented Feb 14, 2020

No also for the simple damped SCF it did not "converge".

@antoine-levitt

This comment has been minimized.

Copy link
Member

antoine-levitt commented Feb 15, 2020

Isn't that because the callback tolerance was too tight? Let's discuss next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.