-
Notifications
You must be signed in to change notification settings - Fork 112
[WIP] LAMMPS Flows #1185
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?
[WIP] LAMMPS Flows #1185
Conversation
…cture, copied over all the work done on atomate2-lammps (by @ml-evs and @gbrunin at Matgenix). The basic functions have been implemented, the actual tasks to be done are generating the right input sets for a wide range of lammps calculations that can be done, and to write a task doc that can handle the outputs of these calculations. Will update the run.py once I get it to work with a complied version of lammps for a simple test case.
…, run_lammps can now be executed
… are probably better expressed in the LAMMPS_CMD, which is specified through the environment's atomate2.yaml file.
…o MDA via a builder
…tter output processing into the taskdoc
…rs from pmg, better handling of inputs to the makers. TODO: make init more readable, and allow for better management of how upstream generators call base set generator
Update atomate2
… based on atomate2.ase.utils.TrajectoryObserver. Also accounted for reading in molecules and saving as a trajectory.
…s to json files for easier access, added utility funcs to process settings dicts
…ings to allow restart keyword to be provided in template
…and langevin/berendsen for now. Added nph as a thermostat too.
…for langevin, need for nve integrator for nvt/npt with non-nose-hoover
… in update_settings function
…ded template and for minimization calcs
… take in TaskState and StoreTrajectoryOption objects from emmet for consistency
…etGenerator in pmg
Updated test files (not the actual tests) to be consistent with new pmg input sets
Removed all instances of the original LammpsInterchange (too difficult to configure correctly), and removed the templates since they've been moved to pymatgen
…into lammps_flows_templated
@vir-k01 Thank you! Before I check out the code in more detail, a naive question: |
@JaGeo Yes, that's a very valid question. I personally haven't used the python interface to LAMMPS much, but from what I know of it, it's entirely equivalent to just writing templates since it does not provide actual objects to use (other than the cmd line runner). I'm sure the lammps python interface has its uses, but in the context of this PR though, I think using templated input files offers a lot more flexibility, since anything that's not a simple NVT/NPT MD is difficult to convert into a structured input set. At least this way, the user can prepare their input file using whatever approach they're comfortable with, whether that be the pymatgen interface to lammps, or the native python lammps interface, or the ASE interface, or just directly writing the file out in a text editor; and pipe that input file into the flows here. |
@vir-k01 Thank you very much for the answer! That sounds very good! |
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 @vir-k01,
thanks a lot for all this work.
Since we are planning to use this I have made a first review of the code and left some comments.
I have already mentioned it in the comments, but my main concern is about how to handle the connection between different jobs. At the moment it seems that the only automation allowed would be passing the output Structure of one Job to the next one. However, in this kind of MD simulations seems that all the additional information (like velocities and thermostat information) would be necessary to have a meaningful connetion.
One potential solution would be to use the "restart" feature in LAMMPS. From a quick test, the size of the restart file generated (or of the "data" file from the write_data
) is realtively small (~50KB for a system with ~400 atoms). It may be an idea to always write the restart at the end, so that the jobs could always be composable, or alternatively at least ease the addition of write_restart
/write_data
to the input file in case one wants to join jobs.
Is there any other way to better ensure the transfer of the required information between two different execution of LAMMPS?
import numpy as np | ||
|
||
|
||
class LammpsNVESet(BaseLammpsSetGenerator): |
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 generators should probably be made as dataclasses. Otherwise all the attributes below will be seen as class attributes, instead of instance attributes.
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.
Oh shoot, I did not realize that. I wrote out the init functions this way for the core set generators to allow the user to provide keywords that they normally would (such as temperature, nsteps etc for NVT) without having to create a LammpsSettings object beforehand. I can make this change but that'll require writing out an init function regardless since this can't be moved into the post_init?
Lammps input set for NVE MD simulations. | ||
""" | ||
ensemble : MDEnsemble = MDEnsemble.nve | ||
settings : dict = {} |
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.
Only dictionary? Why not also LammpsSettings
as in the base class?
""" | ||
Lammps input set for NVE MD simulations. | ||
""" | ||
ensemble : MDEnsemble = MDEnsemble.nve |
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 point to this argument, or at least having it as atomate2.ase.md.MDEnsemble
? I could understand if there was a base class with common code among LammpsNVESet
, LammpsNVTSet
and just set the value in ensamble
. But here the class is called LammpsNVESet
, is there any other meaningful value to set for ensamble
here? The same for the other generators.
ensemble : MDEnsemble = MDEnsemble.nve | ||
settings : dict = {} | ||
|
||
def __init__(self, **kwargs): |
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.
probably making these generators dataclasses and replace this with a __post_init__
will remove the need for a good part of the code in the __init__
?
src/atomate2/lammps/files.py
Outdated
''' | ||
def __init__(self, dumpfile, store_md_outputs : StoreTrajectoryOption = StoreTrajectoryOption.NO, read_index: str | int = ':') -> None: | ||
self.store_md_outputs = store_md_outputs | ||
self.traj = read(dumpfile, index=read_index) if isinstance(read_index, str) else [read(dumpfile, index=read_index)] |
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.
This is not entirely correct. In fact, if read_index
containts an integer index in the form of a string (for example "1"
) read
still returns a single Atoms
object. I understand that this is a particular case, but it is still make the simple check with isinstance(read_index, str)
potentially incorrect.
… default init, ruff
…topology info, fix return type in Dumpconverteer
sync to main
Update to main
Summary
This is an effort that picks up from #173 to incorporate workflows to run LAMMPS in atomate2. Quite a bit of the initial code was taken from the atomate2-lammps add-on (here) written by @ml-evs and @gbrunin. The input set generator and templates have been moved to pymatgen.io.lammps, and a concurrent PR has been opened to integrate those into pymatgen. Also tagging in @esoteric-ephemera who helped structure some of the code here, and @davidwaroquiers for their interest in this PR.
TODO