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

Pin MACE calculator version, add missing metadata to AseStructureTaskDoc #1119

Merged
merged 8 commits into from
Feb 11, 2025

Conversation

esoteric-ephemera
Copy link
Contributor

@esoteric-ephemera esoteric-ephemera commented Feb 10, 2025

Fixes three issues:

  • MACE calculator previously relied on default being MACE-MP-0, whereas the default has now changed to an updated model, MACE-MPA-0
  • The AseStructureTaskDoc and ForceFieldTaskDocument were missing some metadata fields when constructed from a relax job. These are just convenience things like number of elements, symmetry, etc. but should be populated
  • Expands the number of forcefield flows with a from_force_field_name method

The MLFF option MACE still points to MACE-MP-0, whereas specifying:

  • MACE_MPA_0 selects MACE-MPA-0
    -MACE_MP_0B3 selects MACE-MP-0b3

I think we want to avoid creating an MLFF forcefield interchange library a la LibXC, so I'm happy to remove the new MLFF values. Also, because MACE-OMat is ASL licensed, don't want to include this as an MLFF by default

@@ -12,7 +12,8 @@
class MLFF(Enum): # TODO inherit from StrEnum when 3.11+
"""Names of ML force fields."""

MACE = "MACE"
MACE = "MACE" # This is MACE-MP-0-medium
MACE_MPA_0 = "MACE_MPA_0"
Copy link
Member

@JaGeo JaGeo Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As there are now 3 different MACE MP 0s, should we use 3b? For phonons, it is a lot better than the initial one, for example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added 0b3 as well. Avoiding Omat for license reasons. Also added a from_force_field_name method to the phonon and QHA makers to allow easier instantiation of these from a str

@JaGeo
Copy link
Member

JaGeo commented Feb 10, 2025

@esoteric-ephemera thank you!

I am still thinking about a few points:

  • i am currently not sure how much better the alexandria model will be but it is the new default in mace
  • users might be confused when we do not use the same default as in mace
  • we should likely use the same nomenclature as in mace as users might also be confused otherwise
  • we should keep the tests with mace-mp-0 (the first one)

What do you think?

@esoteric-ephemera
Copy link
Contributor Author

@JaGeo these are good points, they merit some discussion. The default in mace_torch is going to change over time as new models are trained. @janosh can comment on that if I'm wrong

I primarily want to avoid a situation where workflows aren't reproducible using the same settings. Adding a MACE_MP_0 MLFF that is identical to MACE, and throwing a warning whenever MACE is specified might be the way to go

@janosh
Copy link
Member

janosh commented Feb 10, 2025

Adding a MACE_MP_0 MLFF that is identical to MACE, and throwing a warning whenever MACE is specified might be the way to go

agreed, that sounds good 👍

@JaGeo
Copy link
Member

JaGeo commented Feb 11, 2025

@JaGeo these are good points, they merit some discussion. The default in mace_torch is going to change over time as new models are trained. @janosh can comment on that if I'm wrong

I primarily want to avoid a situation where workflows aren't reproducible using the same settings. Adding a MACE_MP_0 MLFF that is identical to MACE, and throwing a warning whenever MACE is specified might be the way to go

Agreed! This sounds very good! Backward-compatibility is extremely important as well!

@JaGeo
Copy link
Member

JaGeo commented Feb 11, 2025

@esoteric-ephemera thank you very much! This is great! I will merge!

@JaGeo JaGeo merged commit e6231ad into materialsproject:main Feb 11, 2025
15 checks passed
@esoteric-ephemera esoteric-ephemera deleted the mlff_issues branch February 11, 2025 16:26
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.

3 participants