ACEsuit / mace

MACE - Fast and accurate machine learning interatomic potentials with higher order equivariant message passing.
Other
413 stars 157 forks source link

fix incorrect conversion of forces from calculator to atoms.arrays for finetuning pbe mp replay head #409

Closed bernstei closed 1 month ago

bernstei commented 2 months ago

Actually store multihead fine-tuning pbe mp forces in Atoms.arrays, rather than incorrect current storage in Atoms.info

Also, store configs selected when multihead fine-tuning in local, tag dependent filename, rather than fixed filename in ~/.cache/mace

bernstei commented 2 months ago

Note - I'm not sure it's ever valid to have an Atoms.arrays with value None, at least in standard ASE, so the except clause may be wrong. However, I'm not sure what exactly later parts of the code, that use these fields, are expecting if the forces are missing for a config.

Also, it's bad practice to except completely arbitrary Exceptions, since they don't distinguish expected things like the property missing from other, unrelated bugs. The ASE exception if the property wasn't read from the file (i.e. isn't in the SinglePointCalculator) is ase.calculators.calculator.PropertyNotImplementedError

ilyes319 commented 2 months ago

Thanks for that. Don't you think cache is better for nodes that do not have GPU access, to prevent having to re run on the head node each time?

bernstei commented 2 months ago

Thanks for that. Don't you think cache is better for nodes that do not have GPU access, to prevent having to re run on the head node each time?

Cache seems to be in home directory anyway (at least it is on my setup, ~/.cache/mace), so not really any different in terms of what disk is being used. Also, I think conceptually it's not great, because that data doesn't get cached - those files are regenerated each time, right? If not, it needs to be much more carefully named to make sure wrong data doesn't get reused. It is a temporary sort of thing, so you could use something like TMPDIR, but if that ends up defaulting to /tmp space could be limited (if we ever want to include all of mp, for example).

What do you think about passing them in memory (e.g. list(Atoms)), rather than as files?