atomate2 icon indicating copy to clipboard operation
atomate2 copied to clipboard

BUG: mismanagement of `MLFF` enum in `ase_calculator`

Open gpetretto opened this issue 7 months ago • 5 comments

Describe the bug This is probably a minor issue, since most of the ase calculators are managed through subclasses of the ForceFieldMDMaker, however the ase_calculator does not work as expected from the definition of the API, since passing an MLFF instance is not properly recognized. The reason is this check: https://github.com/materialsproject/atomate2/blob/0e50e0727e852a65dccb8216c6a0c47cf8ae50ca/src/atomate2/forcefields/utils.py#L45

Since str(MLFF.MACE) gives 'MLFF.MACE', this check translates in something like MLFF.MACE in ["MLFF.FACE", ...] which resolves to False.

Overall the ForceFieldMDMaker works because it always converts MLFF to a string: https://github.com/materialsproject/atomate2/blob/0e50e0727e852a65dccb8216c6a0c47cf8ae50ca/src/atomate2/forcefields/md.py#L160

I am not opening a PR because I am not sure how you would rather address this. In principle changing the code so that it accepts the string "MACE" instead of "MLFF.MACE" may be a backward incompatible change.

To Reproduce

from atomate2.forcefields import MLFF
from atomate2.forcefields.utils import ase_calculator

ase_calculator(MLFF.MACE) # raises an error: ValueError: Could not create ASE calculator for MLFF.MACE.
ase_calculator("MACE") # raises an error: ValueError: Could not create ASE calculator for MLFF.MACE.
ase_calculator(str(MLFF.MACE)) # works
ase_calculator("MLFF.MACE") # works

gpetretto avatar May 27 '25 15:05 gpetretto

@gpetretto I am also not entirely sure how to solve in the best way.

Can we accept both "MLFF.MACE" and "MACE"?

JaGeo avatar May 28 '25 08:05 JaGeo

Probably that's a good solution. In a sense I imagine this can be considered a bug, as I expect that when it was implemented it was thought to be only "MACE". so one can also decide of switching entirely to that. However, I would be more considerate in not breaking previously existing code.

Note also that MLFF have this TODO: https://github.com/materialsproject/atomate2/blob/0e50e0727e852a65dccb8216c6a0c47cf8ae50ca/src/atomate2/forcefields/init.py#L13 And if switched to StrEnum it will also become that str(MLFF.MACE) will be converted to just "MACE".

gpetretto avatar May 28 '25 10:05 gpetretto

@gpetretto thanks for catching this - I'm currently working on some other fixes / changes for the forcefields and will add this in

esoteric-ephemera avatar Jun 04 '25 23:06 esoteric-ephemera

Thanks!

gpetretto avatar Jun 05 '25 14:06 gpetretto

@gpetretto would you mind checking out #1220 and seeing if ase_calculator is working as you expect?

esoteric-ephemera avatar Jun 13 '25 18:06 esoteric-ephemera