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

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

Merged
merged 1 commit into from
May 6, 2024

Conversation

bernstei
Copy link
Collaborator

@bernstei bernstei commented May 5, 2024

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

rather than incorrect current storage in atoms.info

store configs selected when multihead fine-tuning in local, tag
dependent filename, rather than fixed filename in ~/.cache/mace
@bernstei
Copy link
Collaborator Author

bernstei commented May 5, 2024

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
Copy link
Contributor

ilyes319 commented May 5, 2024

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
Copy link
Collaborator Author

bernstei commented May 5, 2024

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?

@ilyes319 ilyes319 merged commit a9681fe into multi-head-interface May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants