-
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 |
|
Oop, sorry I missed that, now changed! |
|
This all looks OK to me, I'll have to see why the approx NEB tests are failing now @JaGeo I agree that it's not great to have this bifurcation of similar MLFF libraries, I'm also not sure how best to avoid that. If you want, we could try to iterate a bit on this and the multi ASE makers you've been working on |
|
@esoteric-ephemera Yeah. For me, it is also unclear how to integrate torchsim into other property workflows. I think it would be worth thinking about this a bit more |
|
|
||
|
|
||
| @dataclass | ||
| class TorchSimIntegrateMaker(Maker): |
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.
Why is it called "Integrate" and not MD?
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.
The TorchSim integrate function also supports some Monte Carlo methods, so it's not strictly limited to MD
|
There is another point. Even if we merge everything in this state, I think we definitely need some new documentation here as well. What to install, how to use it in the best way etc. Some examples. |
|
I have made up my mind. If one can show that with this current schema, one can plug TorchSim Flows into larger workflows (e.g., the phonon workflow where a batch mode already exists), I am completely fine with merging. I think this is a basic requirement that such a new schema should fulfill. (plus the documentation, of course) |
|
Thanks for the reviews, I understand your concerns about the bifurcation of MLFF libraries.
Happy to add this sort of documentation. I can write a tutorial to walk through the usage.
I'll explore this over the next few days! Would you mind pointing me to the batched Phonon Maker? I am looking at class BasePhononMaker(Maker, ABC):
def make(
self,
structure: Structure,
prev_dir: str | Path | None = None,
born: list[Matrix3D] | None = None,
epsilon_static: Matrix3D | None = None,
total_dft_energy_per_formula_unit: float | None = None,
supercell_matrix: Matrix3D | None = None,
) -> Flow:
``` |
|
Hi @orionarcher,
All other suggestions sound good. |
|
I did some digging on the PhononMaker and found the following TaskDoc fields accessed through different Makers. Thus the task doc must include: task_doc.structure
task_doc.output.energy
task_doc.dir_name
task_doc.uuid
task_doc.output.all_forces
# and for born charges
task_doc.calcs_reversed[0].output.epsilon_static
task_doc.calcs_reversed[0].output.outcar["born"]The first five seem doable and I am happy to add them (and let me know if it seems like I am missing something). TorchSim, however, doesn't currently support Born Charges, so we would be able to support the born_maker argument with TorchSim. I propose refactoring the TaskDoc to expose those arguments in the same positions as the ForceFieldTaskDoc but leaving out the Born charges for now. Does that, along with the addition of more docs and a tutorial, seem like a reasonable path forward? Once the batched schema is merged in, it should then be quite straightforward to align the schemas. Another thought is that we could make the structure inputs and outputs |
|
To me, this sounds very good! Thanks! We do not have any born charges in the force-field based MLIPs as well. This will be fine. We might have them from ML at some point in the future. |
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.