search

hackingmaterials / atomate

atomate is a powerful software for computational materials science and contains pre-built workflows.
https://hackingmaterials.github.io/atomate
Other
245 stars 175 forks source link

remove MaxForceErrorHandler #776

Closed JonathanSchmidt1 closed 1 year ago

JonathanSchmidt1 commented 1 year ago

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 https://github.com/materialsproject/custodian/issues/103 . From checking the custodian code I assume it should be replaced with the DriftErrorHandler or just deleted but I am not sure.

MichaelWolloch commented 1 year ago

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, https://github.com/materialsproject/custodian/commit/d322affa800859a8fd1657528fc5f19542ca0580 .

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 (https://github.com/materialsproject/custodian/pull/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 commented 1 year ago

I will also include the parameter change for R2SCAN as mentioned recently by @arosen93 in https://github.com/hackingmaterials/atomate/issues/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.

MichaelWolloch commented 1 year ago

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 commented 1 year ago

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