BUG: mismanagement of `MLFF` enum in `ase_calculator`
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 I am also not entirely sure how to solve in the best way.
Can we accept both "MLFF.MACE" and "MACE"?
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 thanks for catching this - I'm currently working on some other fixes / changes for the forcefields and will add this in
Thanks!
@gpetretto would you mind checking out #1220 and seeing if ase_calculator is working as you expect?