-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add MACE RelaxMaker and StaticMaker #611
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #611 +/- ##
==========================================
- Coverage 75.54% 75.49% -0.05%
==========================================
Files 83 83
Lines 6808 6839 +31
Branches 1008 1010 +2
==========================================
+ Hits 5143 5163 +20
- Misses 1351 1363 +12
+ Partials 314 313 -1
|
tests/forcefields/test_jobs.py
Outdated
# FIXME - brittle due to inability to adjust dtypes in M3GNetRelaxMaker | ||
torch.set_default_dtype(torch.float32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need these? I think
potential_kwargs={"default_dtype": "float64"},
should do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't see where the dtypes can be set in chgnet codebase so need to do globally, issue is MACE does it globally to 64 so tests using chg/m3g after mace jobs test were failing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then this would affect production runs as well. It would be great to make is afe to use different types of potential in one go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Pinging @ilyes319 and @davkovacs to see if this can be solved on MACE's side without changing global state?
For now, we'll wrap the test in try/finally
and revert the torch.dtype
at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to make it safe to use different types of potential in one go.
@JaGeo Strongly agree. I think we can figure something out with Ilyes and David.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes dtype conversion has haunted me for a long time...
…rbed into potential_kwargs fix tests fix doc string
in MACE tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks @CompRhys! 👍
Summary
Adds MACE RelaxMaker and StaticMaker, adds a 2Mb model for testing, could adopt MACE pattern and train a fixture in the test
Additional dependencies introduced (if any)
https://github.com/ACEsuit/mace added to
strict
andforcefield
Checklist
The easiest way to handle this is to run the following in the correct sequence on
your local machine. Start with running black on your new code. This will
automatically reformat your code to PEP8 conventions and removes most issues. Then run
ruff.
Run ruff on your code.
type check your code.
Note that the CI system will run all the above checks. But it will be much more
efficient if you already fix most errors prior to submitting the PR. It is highly
recommended that you use the pre-commit hook provided in the repository. Simply run
pre-commit install
and a check will be run prior to allowing commits.