Add schema for atomate2 classical_md workflows #975
Conversation
…atomate2 workflows.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #975 +/- ##
=======================================
Coverage 90.01% 90.01%
=======================================
Files 138 138
Lines 13226 13226
=======================================
Hits 11906 11906
Misses 1320 1320 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I know this is already merged, but as requested in materialsproject/atomate2#782 (comment) I've left some minor comments / suggestions, mostly relating to naming and documentation of various field names.
All are just suggestions, please choose to act on them or not as you judge best. In general I think you've captured all the important aspects that come to mind.
| dir_name: Optional[str] = Field( | ||
| None, description="The directory for this VASP task" | ||
| ) |
| None, description="Molecules within the box." | ||
| ) | ||
|
|
||
| forcefield: Optional[str] = Field(None, description="forcefield") |
There was a problem hiding this comment.
It seems like most of the field names have used a convention of underscores where there would be multiple words. Should this be force_field for consistency?
| interchange: Optional[str] = Field( | ||
| None, description="Final output structure from the task" |
There was a problem hiding this comment.
I think it would make more sense to call this structure or trajectory or system or something along those lines (not everyone will be familiar with what an "interchange" is . Or if nothing else, perhaps include as an alias?
| calcs_reversed: Optional[list] = Field( | ||
| None, | ||
| title="Calcs reversed data", | ||
| description="Detailed data for each VASP calculation contributing to " |
| description=( | ||
| "The interval for saving simulation state. For no state, set to 0." | ||
| ), |
There was a problem hiding this comment.
Does this carry units? If so, would be nice to specify them in the docstring
| description=( | ||
| "The interval for saving trajectory frames. For no trajectory, set to 0." | ||
| ), |
There was a problem hiding this comment.
Same comment about units. If there are no units and this is the number of timesteps, that would also be good to specify.
| steps: Optional[list[int]] = Field( | ||
| None, description="Steps where outputs are reported" | ||
| ) |
There was a problem hiding this comment.
n_steps in input vs. steps in output might be slightly confusing. Would it make sense to use a name like save_every_steps or something to be a little more clear?
| elapsed_time: Optional[float] = Field( | ||
| None, description="Elapsed time for the calculation" | ||
| ) |
There was a problem hiding this comment.
Is it possible to specify units here? Or does it depend on how the calc input is set up?
|
|
||
| has_openmm_completed: Optional[Union[TaskState, bool]] = Field( | ||
| None, description="Whether OpenMM completed the calculation successfully" | ||
| ) |
There was a problem hiding this comment.
This field is great to have. I'm not (yet) familiar enough with atomate2 to know if there's a more standard way to report whether the executable has finished. For example you have state in ClassicalMDTaskDocument
If there is not a common standard emerging yet, I'd say it's fine as is.
| class ClassicalMDTaskDocument(BaseModel, extra="allow"): # type: ignore[call-arg] | ||
| """Definition of the OpenMM task document.""" | ||
|
|
There was a problem hiding this comment.
Would it make any sense to have an ensemble field here? If I recall correctly OpenMM doesn't necessarly enforce a strict ensemble so maybe / maybe not, but thinking generally about classical MD, that's the only significant piece of info that I don't see reflected in the schema.
|
Thanks @rkingsbury! I'll integrate this feedback into my next PR. |
This adds in schema for the nearly complete
atomate2.classicalworkflows in this PR.The PR is just pydantic schema and no new functionality or dependencies are added.
Contributor Checklist