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

Generalize forcefields for generic ASE calculator support #940

Merged
merged 112 commits into from
Sep 25, 2024

Conversation

esoteric-ephemera
Copy link
Contributor

@esoteric-ephemera esoteric-ephemera commented Aug 2, 2024

This is a refactor to better support generic ASE calculators in atomate2, as well as molecules with ASE calculators.

Major changes:

  • schemas / utils have been ported from the forcefields library to a separate ase library
  • Forcefield-specific jobs now inherit from ASE base classes, such as the ForceFieldTaskDocument from the base AseStructureTaskDoc class
  • Generic, Structure, and Molecule-specific ASE task doc classes
  • Example ASE jobs using the Lennard-Jones and GFNn-xTB (TBLite) calculators
  • The ForcefieldResult and ForcefieldObject classes alias ASE parent classes + deprecation warning
  • Move MD makers from forcefields to ase
  • Lazy load ase .MolecularDynamics objects
  • XDATCAR support for trajectory output (sometimes useful)

Most of the other moved classes are internals for atomate2, so I don't expect many important consequences there

@esoteric-ephemera esoteric-ephemera marked this pull request as draft August 2, 2024 22:31
@JaGeo
Copy link
Member

JaGeo commented Aug 3, 2024

@esoteric-ephemera Great!
Even though the outside usage of ForcefieldResult might be limited. Let's maybe still alias - just in case.

@esoteric-ephemera
Copy link
Contributor Author

Hi @JaGeo, restored the ForcefieldObejct and ForcefieldResult objects for now with a deprecation warning

@utf
Copy link
Member

utf commented Aug 6, 2024

Hi @esoteric-ephemera, this is really nice to see in atomate2. Your implementation looks great to me. Would you be also happy to add static makers?

In terms of your todos:

Replicating all "standard" flows (elastic, eos, gruneisen, phonon) with a base ASE calculator class?

I'm not sure if this makes sense? We'd still need to make subclasses for specific implementations anyway, so I don't think a base implementation would add anything?

Tests for the ASE calculators currently in atomate2.ase.jobs (having some trouble getting TBLite working on an M1 Mac)

This would be great if you can figure it out.

@esoteric-ephemera
Copy link
Contributor Author

esoteric-ephemera commented Aug 7, 2024

I'm not sure if this makes sense? We'd still need to make subclasses for specific implementations anyway, so I don't think a base implementation would add anything?

Sounds good, I'll just port the MD maker over to ase from forcefields and call it

Static makers have been added + tests

JonathanSchmidt1 added a commit to JonathanSchmidt1/atomate2_other_forcefields that referenced this pull request Aug 8, 2024
@utf
Copy link
Member

utf commented Sep 24, 2024

Hi @esoteric-ephemera. Thanks again for this. Is it ready to merge?

utf pushed a commit that referenced this pull request Sep 24, 2024
* starting point for input set docs

* remove sentence

* correct InputGenerator name in figure

* adjust figure size

* reflect changes of vasp inputsets now in pymatgen

* fix linting

* change torchdata version according to #940

* change torchdata version according to #940

* Update pyproject.toml

resolve conflict with changed matgl version
@esoteric-ephemera
Copy link
Contributor Author

esoteric-ephemera commented Sep 24, 2024

Hi @utf should be ready to merge provided the tests pass (need to make sure these still run OK after resolving merge conflicts with OpenMM)

AseStructureTaskDoc or AseMoleculeTaskDoc
"""
return AseTaskDoc.to_mol_or_struct_metadata_doc(
getattr(self.calculator, "name", self.calculator.__class__),
Copy link
Member

Choose a reason for hiding this comment

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

i think this line will fail pydantic validation if a calculator should ever not have a name. __class__ is not a string, we want the __name__ attribute

https://github.com/materialsproject/atomate2/pull/940/files#diff-9eab185b405c88913d9a52dc90990b06235a6955a91c6c06ccbc899a4e3ec5b7R204-R207

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

@janosh janosh enabled auto-merge (squash) September 25, 2024 14:07
@utf
Copy link
Member

utf commented Sep 25, 2024

Thanks @esoteric-ephemera, this is very exciting to have!

@JaGeo
Copy link
Member

JaGeo commented Sep 25, 2024

In that context: should we document this and a few other of our new workflows that are not VASP-based more clearly? I think a lot of people have interest in this general interface but might not discover it easily

@janosh janosh merged commit fdb4894 into materialsproject:main Sep 25, 2024
9 checks passed
@esoteric-ephemera
Copy link
Contributor Author

@JaGeo agreed, this should probably go in the docs for clarity. Also happy to add to the working paper

@JaGeo
Copy link
Member

JaGeo commented Sep 25, 2024

@JaGeo agreed, this should probably go in the docs for clarity. Also happy to add to the working paper

Personally, I think it should be also in the paper. Thoughts, @utf ?

@utf
Copy link
Member

utf commented Sep 25, 2024

If you're happy to add to the paper, I think it should be highlighted.

@JaGeo
Copy link
Member

JaGeo commented Sep 25, 2024

And thanks @esoteric-ephemera (bit confused today 🙈 or maybe as usual)

@utf utf added the feature A new feature being added label Sep 26, 2024
hrushikesh-s pushed a commit to hrushikesh-s/atomate2 that referenced this pull request Nov 16, 2024
* starting point for input set docs

* remove sentence

* correct InputGenerator name in figure

* adjust figure size

* reflect changes of vasp inputsets now in pymatgen

* fix linting

* change torchdata version according to materialsproject#940

* change torchdata version according to materialsproject#940

* Update pyproject.toml

resolve conflict with changed matgl version
hrushikesh-s pushed a commit to hrushikesh-s/atomate2 that referenced this pull request Nov 16, 2024
…roject#940)

- Add ASE base calculator classes from forcefield library
- Add tests for ASE calculators, refactor forcefields
- Add static makers
- Add tblite dependence to ASE in toml
- Migrate forcefield MD base class to ASE
- Add barebones tests for ASE MD jobs
- Generalize to allow molecules in ASE calculations
- Make tblite dependence optional
- Move forcefield strict version pins to separate toml block
- Refactor naming, enumify and lazy load MD dynamics modules
- Make Ase{Structure,Molecule}TaskDoc construction a single function
- Change ASE and forcefield result from dict to BaseModel
- Ensure ASE and forcefield result docs are subscriptable
- Add "ionic_steps" to DATA_OBJECTS
- Add 'elapsed_time' and 'tags' to ASE schemas
- Add timing to ASE runs
- Define basic ASE job schema; remove task_doc_kwargs from ASE in favor of explicit kwargs
- Revise ionic_step storage (only for MD)
- Add option to pass forcefield name as str without MLFF prefix or as enum
- Add deprecation warnings to all predefined MLFF makers (static, relax, MD)
- Correct typing of atoms in AseRelaxer
- Add tblite to optional ase-ext and strict requirements
- Remove dependence on deprecated forcefield makers
- Make default calc kwargs and ensure these are loaded when not specified by user
- Remove Frechet cell filter safety checks
- Make MD ensembles an enum
- Move ASE tests to separate test run
- Add ASE to phonon supported codes, enforce string literal in BasePhononMaker
- Fix potential Pydantic validation error if Ase(MD|Relax)Maker.calculator has no name

---------

Co-authored-by: orionarcher <[email protected]>
Co-authored-by: Janosh Riebesell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ase Atomic Simulation Environment feature A new feature being added forcefields Forcefield related refactor Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants