-
Notifications
You must be signed in to change notification settings - Fork 55
add support for graph-pes models
#118
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
Conversation
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.
Hi @jla-gardner, thanks for the PR! I would be very happy to have this interface in TorchSim.
For added context, I want to point you to our current posture on model interfaces, just to share how we are thinking about supporting models over time.
I've included a few comments to help get the tests working and make sure that the code correctly interfaces with TorchSim.
Thanks for the extremely quick response! Yes this is slightly a-typical as being an interface to an interface. Going through all your comments point by point now. |
|
Hi guys - I'm looking into the test failures from the I'm seeing that the tests are only failing for the I've pushed a change to that effect now as a test |
Yes I did reduce the MACE tolerances due to the SiO2 rattling test breaking. It was on my TODO list to dig a little bit deeper on that incase it actually is a real difference vs numerical non-determinism that might be exaggerated by the MACE architecture. It's worth noting that I do not see that rattling test breaking locally when running on M3 macbook. |
|
I did some tests locally and it seems that the |
|
All tests pass locally on my MacBook M2 too for what it's worth. Do you have a preference between reducing the rattling amplitude and using a larger model (as suggested by @abhijeetgangan!) for testing to keep this test passing? |
|
If the larger models don't have a big overhead in testing then I would probably use those. @CompRhys What do you think? |
biggest MACE still smaller than smallest Fairchem so I would suspect it wouldn't add too much time to the tests. I would be happy to swap to testing |
|
|
updated to use |
|
Looking into these continuing errors on the |
|
Hi guys, after some investigating I have found that I have ensured that the |
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.
Looks good to me (one tiny nit), thanks for the quick edits @jla-gardner!
I pushed a small change to docs.yml that will test the docs build in the branch. That should be in main anyway and I'd like to check it before merging.
tests/models/test_graphpes.py
Outdated
| calculator_fixture_name="ase_mace_calculator", | ||
| sim_state_names=consistency_test_simstate_fixtures, | ||
| # see test_mace.py for similar issue | ||
| rtol=6e-4, # FIXME: unclear why this needs to be so high for mace. # noqa: FIX001 |
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.
Could you take out the comment if this has been resolved or add a note explaining your findings?
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.
Tentatively removed the high rtol requirements in the latest commit to see whether this is actually completely resolved. I fear that it won't be, as graph-pes is just passing data straight through to mace-torch, and so if the base MACE model requires these high rtols in test_mace.py, we probably need them here too.
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.
Okay, that didn't work. Reverting now and adding comments to this effect
|
One more thing, could you modify |
|
To get the docs build to work you'll need to add |
Added this but have now managed to break the docs build further down the track with the cryptic error Clearly my graph-pes install has made the docs fall over somewhere - trying to reproduce locally. |
|
Hmmm, very strange. @janosh has encountered that error when trying to build the docs locally, too. He might have input. I'll take a look too. Ignore the 5.3 failure, thats intermittent. |
I've reproduced locally - I think there is a subtle incompatibility between latest versions of graph-pes and mace-torch. Will update with a new minimum requirement for |
Updated. docs are now building locally with the newly pinned graph-pes version. |
|
Thanks for the contribution @jla-gardner. Good to merge? |
Absolutely 😄 thanks once again for the sterling work with |
Hi there 👋 thanks for the awesome work with
torch-sim😊Summary
I wrote and maintain
graph-pes, a package for creating and training models of the potential energy surface that act on graph representations of atomic structures. It would be amazing if my users could make use oftorch-simto run fast MD in a hassle free manner! (I currently have LAMMPS and ASE calculators which are annoying to set up and ~slow respectively).This PR adds support for using arbitrary
GraphPESModels from the graph-pes package withintorch-sim.I've also added a small tutorial notebook to give a concrete example of how to use the
GraphPESWrapperclass in the docs.Checklist