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

Allow custom mace model by specifying "model" in calculator kwargs" #1017

Merged
merged 16 commits into from
Oct 31, 2024

Conversation

orionarcher
Copy link
Contributor

@orionarcher orionarcher commented Oct 15, 2024

Currently it's only possible to use the mace_mp model. This PR allows users to use a custom model by making the "model" kwarg a path to the model. If model is not a Path, then mace_mp is used, thus this change should be backwards compatible.

EDIT: More context:

mace==0.3.3, the version currently used by atomate2, allows custom mace models in mace_mp. So the current code does not need any changes to use custom models.

mace==0.3.7, the most recent version does not allow custom models in mace_mp, which is a breaking change. This PR will be needed when atomate2 bumps the mace version. When that happens, many tests will likely need fixing.

@orionarcher
Copy link
Contributor Author

More context:

mace 0.3.3, the version currently used by atomate2, allows custom mace models in mace_mp. So the current code does not need any changes to use custom models.

mace 0.3.7, the most recent version does not allow custom models in mace_mp, which is a breaking change. This PR will be needed when atomate2 bumps the mace version. When that happens, many tests will likely need fixing.

@janosh @utf, how does atomate2 intend to evolve the mace dependency over time? They introduced some breaking changes but also useful new features.

@janosh
Copy link
Member

janosh commented Oct 23, 2024

@janosh @utf, how does atomate2 intend to evolve the mace dependency over time? They introduced some breaking changes but also useful new features.

i think the latest version of atomate2 should strive to be compatible with the latest version of mace-torch, even if that means adapting to breaking changes

@orionarcher orionarcher force-pushed the allow_custom_mace_model branch from 3632b26 to fd07d69 Compare October 28, 2024 14:56
@JaGeo
Copy link
Member

JaGeo commented Oct 30, 2024

@orionarcher I will remove the orb stuff here and then merge. There is a separate pull request for the orb part in #1031

@JaGeo JaGeo enabled auto-merge (squash) October 30, 2024 16:25
@JaGeo
Copy link
Member

JaGeo commented Oct 30, 2024

Oh, there seems to be something wrong with the kwargs. Will check later and then merge.

@JaGeo
Copy link
Member

JaGeo commented Oct 30, 2024

@esoteric-ephemera Could you shortly have a look at this failing test? To me, it looks like the symmetry does not change during relaxation with the latest mace version. At least, within the ASERelaxer, the correct "relax_cell" and "fix_symmetry" are set.

@esoteric-ephemera
Copy link
Contributor

@JaGeo sure! Will look at this soon

model = kwargs.get("model")
if isinstance(model, str | Path) and os.path.isfile(model):
model_path = model
calculator = MACECalculator(
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we need to change this to:

device = kwargs.get("device") or "cpu"
calculator =  MACECalculator(
    model_paths=model_path,
    device = device,
    **kwargs
)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. One needs to remove the "device" key from the kwargs. Otherwise, it will complain that it is set twice. Will do that.

@esoteric-ephemera
Copy link
Contributor

OK so if the symmetry of a cell is fixed, we certainly should assert that the initial and final symmetries of the cell are the same

But if we don't fix the symmetry, there's a distinct possibility that the symmetry of the cell remains the same. It could change, but unless the force convergence criterion is made tighter, there's a decent chance the structure barely changes

For example, with the previous setting of fmax = 0.04 eV/Å, the atoms don't move much. With that tightened to 0.01 eV/Å, the symmetry of the cell changes during relaxation

Also worth noting that it seems like ASE's FixSymmetry isn't holding for more stringent force convergence criteria. I pushed fmax to 0.001 eV/Å and the cell symmetry wasn't preserved with fix_symmetry = True

@JaGeo
Copy link
Member

JaGeo commented Oct 31, 2024

@esoteric-ephemera so, you are okay with simply adapting the test, right?

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 66.58%. Comparing base (42bc7b8) to head (1f5e138).

Files with missing lines Patch % Lines
src/atomate2/forcefields/utils.py 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            main    #1017       +/-   ##
==========================================
+ Coverage   4.32%   66.58%   +62.25%     
==========================================
  Files        178      178               
  Lines      12911    12921       +10     
  Branches    1278     1280        +2     
==========================================
+ Hits         559     8604     +8045     
+ Misses     12321     3838     -8483     
- Partials      31      479      +448     
Files with missing lines Coverage Δ
src/atomate2/forcefields/utils.py 68.33% <83.33%> (+68.33%) ⬆️

... and 150 files with indirect coverage changes

@JaGeo
Copy link
Member

JaGeo commented Oct 31, 2024

@esoteric-ephemera @orionarcher Okay, I figured it out. mace_mp has a default floating type precision of float32. If we set this in the MACECalculator, we get the same results as before. Will merge this now. (Sorry, took a little longer for me to see this even though it is kind of obvious. What else besides the floating type precision should influence such a test when we do not change the MACE version ...)

@JaGeo JaGeo disabled auto-merge October 31, 2024 07:54
@JaGeo JaGeo enabled auto-merge (squash) October 31, 2024 07:58
@JaGeo JaGeo merged commit 9c4447c into materialsproject:main Oct 31, 2024
15 checks passed
@JaGeo
Copy link
Member

JaGeo commented Oct 31, 2024

@utf @janosh I have one question regarding backward compatibility: I decided to go with the new default (floating point precision of the model that one trained). This might influence the results a bit, but I think it is smarter than setting float32 by default. If we should change this, let me know.

@esoteric-ephemera
Copy link
Contributor

@JaGeo good with adapting the test as needed but good to know it was a dtype issue

@utf utf added the house-keeping Formatting and code quality tweaks label Nov 12, 2024
hrushikesh-s pushed a commit to hrushikesh-s/atomate2 that referenced this pull request Nov 16, 2024
…aterialsproject#1017)

* allow custom mace model by specifying "model" in calculator kwargs"

* fix error in trying to turn None into path

* Add support for ORB model

* Specify more dependencies

* remove orb implementation

* add line

* add line

* remove device

* set device

* fix set device

* fix test

* fix linting

* restore test

* remove os and rely on pathlib only

---------

Co-authored-by: J. George <JaGeo@users.noreply.github.com>
Co-authored-by: JaGeo <janinegeorge.ulfen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
house-keeping Formatting and code quality tweaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants