-
Notifications
You must be signed in to change notification settings - Fork 133
Add support for Torchsim #1335
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
base: main
Are you sure you want to change the base?
Add support for Torchsim #1335
Conversation
esoteric-ephemera
left a comment
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.
Some general comments besides the ones on specific lines:
- The TS prefixing in makers, enums, etc. is confusing given that TS is probably more familiar as "transition state" than "torchsim" - can you change this to
TorchSim? - Are you envisioning this being added as a separate (as its currently implemented) or additive module to the existing forcefields stuff? If the latter, the schemas would have to be merged for the job outputs
reduce redundancy in initialization logic
Good call.
I think this should be a separate module. It would be really messy to integrate it with the existing forcefields stuff. TorchSim generally expects many -> many calculations (list[structure] -> list[structure]) and has different output files and such. |
|
Encouraging! In that case, let me reframe. It looks like it would be possible, but I anticipate it would be a major headache, one I am reluctant to take on. Though they both run MLIPs, ASE and TorchSim are different software packages with pretty different APIs. I don't see a major reason to have them share schema or logic. The |
|
The motivation should always be the following: |
|
I hear you and I am sympathetic to that argument. In this case, I feel there is a tradeoff between the immediate adoptability of the TorchSim interface and it's overall quality. After looking back and forth at the ASE and TorchSim schema for the past 15 minutes, I don't think it's possible to adapt the TorchSim API to fit the ASE schemas without adding complexity, reducing readability, and making the overall API less natural and maintainable. I would love for users currently using ASE to be able to quickly and reliably switch to TorchSim but it's not clear to me that equating the schemas is the best way to do that. I would be happy to write a transition guide outlining the schema differences and how to transition from ASE -> TorchSim and add it to this PR. |
|
@JaGeo @esoteric-ephemera do you have any idea why the abinit tests might be failing here? This seems unrelated to any changes I made wrt TorchSim? |
|
I think you can safely ignore it, can happen with parallel runners and there should be a fix for this in the next release of abipy |
|
Other than the unrelated failing tests, I think this is ready to merge? The tests are passing locally and I think all the suggestions are addressed. |
|
Bumped a few comments for your consideration; totally OK if you disagree with the suggestions therein |
|
Thanks. That looks already great! One comment: have you tested integrating one of thrse jobs into the phonon maker or a similar one? Could be a test, for example, to ensure schema structure. |
…rial (#3110) ## Summary of Changes I made some changes to the [`atomate2` interface](materialsproject/atomate2#1335) and I want to make sure they are reflected here! Namely, we automatically calculated energy, forces, and optionally stress and put them in the output dict without needing to deal with the trajectory reporter. I also added a section in the docs for torchsim, let me know if it's in the right place / makes sense. I did verify that all the snippets would run. ### Requirements - [x] My PR is focused on a [single feature addition or bugfix](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/best-practices-for-pull-requests#write-small-prs). - [x] My PR has relevant, comprehensive [unit tests](https://quantum-accelerators.github.io/quacc/dev/contributing.html#unit-tests). - [x] My PR is on a [custom branch](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-and-deleting-branches-within-your-repository) (i.e. is _not_ named `main`). Note: If you are an external contributor, you will see a comment from [@buildbot-princeton](https://github.com/buildbot-princeton). This is solely for the maintainers.
|
End to end compatibility testing will have to wait until #1196 is merged. TorchSim only has a batched mode so it won't work with the current PhononMaker. That said, I've done some basic compatibility testing and I think it will be an easy adaption once #1196 is in. I am happy to loop back around and write a test to verify integration once that's in? |
|
@orionarcher I am not sure I agree that #1196 needs to be in. My queries here are mainly to avoid breaking changes in the future. The more we think ahead towards other workflows, the less we have to adapt the schema in the future. @esoteric-ephemera is it for you sufficently similar to the Ase and ForcefieldMakers? |
|
Ok fair enough. Upon further consideration, it's possible to get compatibility by making the TorchSim functions support I appreciate all the feedback and design thoughts on the PR. |
|
|
||
| # Create the phonon flow | ||
| flow = phonon_maker.make(si_structure) | ||
|
|
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.
Is there a reason not to try running 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.
Done! Good catch.
|
@orionarcher thanks! That already looks great! I made one (last) comment. |
|
Good catch, the test was not testing properly. I made the change and it not works with the PhononMaker! |
|
@orionarcher thanks! We now need to fix the tests. They are failing as the installations need to much cache. We might need to do some cache cleaning after certain workflows or come up with a different idea. |
|
Yeah, looks like TorchSim is the straw that broke the camels back. I could split the tests into a separate action but I don't know if that's the most sustainable route. What would you propose as a solution there? Would you like me to take a swing at it or would you like to try a fix? |
|
I don't have a good solution. Separate action sounds good for me for now. I won't be able to do much testing here myself for a couple of weeks. @esoteric-ephemera do you have any opinion here? Also with regard to usage of action time? |
|
I split the TorchSim stuff out into separate tests. It's dependent on I think it should all be good to go now? |
|
Separate workflow action / separate test sounds good to me. Also for the best given the torch version pin needed for some of the MLFFs, as you said @orionarcher Will take another look through and ping you if I see anything, thanks for your work on this so far! |
|
Fine from my side now. Once @esoteric-ephemera is happy, we can merge :) |
Summary
This PR adds support for TorchSim, namely:
integrate,optimizeandstaticfunctionsintegrate,optimizeandstaticjobsNOTE: this PR uses StrEnum's which are not supported in python 3.10, see #1334
Additional dependencies introduced (if any)
Checklist
Work-in-progress pull requests are encouraged, but please put [WIP] in the pull request
title.
Before a pull request can be merged, the following items must be checked:
The easiest way to handle this is to run the following in the correct sequence on
your local machine. Start with running
ruffandruff formaton your new code. This willautomatically reformat your code to PEP8 conventions and fix many linting issues.
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 installand a check will be run prior to allowing commits.