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

remove MaxForceErrorHandler #776

Closed
JonathanSchmidt1 opened this issue Jul 18, 2023 · 4 comments
Closed

remove MaxForceErrorHandler #776

JonathanSchmidt1 opened this issue Jul 18, 2023 · 4 comments
Labels

Comments

@JonathanSchmidt1
Copy link

The forum seems to be down therefore I am posting this bug report here.

Following the tutorial and running
add -l vasp -s optimize_only.yaml -m mp-149 -c '{"vasp_cmd": ">>vasp_cmd<<", "db_file": ">>db_file<<"}'

leads to an issue as the MaxForceErrorHandler was removed from custodian following materialsproject/custodian#103 . From checking the custodian code I assume it should be replaced with the DriftErrorHandler or just deleted but I am not sure.

@MichaelWolloch
Copy link
Contributor

Hi, I am happy to make a PR to address this issue.

The MaxForceErrorHandler has been removed (after being deprecated for quite a while) by @arosen93 in May, materialsproject/custodian@d322aff .

As far as I can tell, this should not be replaced by the DriftErrorHandler, which is doing something different and is already included in the strict handler group of RunVaspCustodian.

So I will make a PR today that just deletes all references to MaxForceErrorHandler I think.

Because it is such a quick fix (and very hard to catch once calculations are done) I will also include the parameter change for R2SCAN as mentioned recently by @arosen93 in #774 . But if this should be a different PR, I am happy to exclude this as well. Maybe @janosh can chime in on this?

I feel that this is a small, easy, but quite important fix. Especially since the newest versions of custodian have introduced some important bug fixes for vasp calculations (materialsproject/custodian#264) and of course other features and fixes.

Note that of now, installing atomate with pip or from github does grab the newest custodian release, which breaks atomate because of the MaxForceErrorHandler issue. So I think there should be a new release of atomate done as well.

@janosh
Copy link
Member

janosh commented Jul 28, 2023

I will also include the parameter change for R2SCAN as mentioned recently by @arosen93 in #774 . But if this should be a different PR, I am happy to exclude this as well. Maybe @janosh can chime in on this?

Feel free to lump into 1 PR. We definitely need a new atomate PR post MaxForceErrorHandler removal.

@janosh janosh added the bug label Jul 28, 2023
@MichaelWolloch
Copy link
Contributor

Thanks @janosh , it is all in #778. I am not sure why the tests are failing in the CI, since they pass on my local machine. Maybe one can try to upgrade the python, pytest, and pluggy versions?

@MichaelWolloch
Copy link
Contributor

This bug was solved in #778, I think this issue can be closed.

@janosh janosh closed this as completed Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants