-
Notifications
You must be signed in to change notification settings - Fork 70
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
Improve trajectory handling for MD #886
Conversation
Thanks @gpetretto ! I've no specific request. This looks great. |
@@ -618,7 +622,7 @@ def from_vasp_files( | |||
strip_bandstructure_projections: bool = False, | |||
strip_dos_projections: bool = False, | |||
store_volumetric_data: Optional[Tuple[str]] = None, | |||
store_trajectory: bool = False, | |||
store_trajectory: Union[bool, str] = False, |
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.
Instead of mixing bool
and str
inputs to support a third "partial" option, I would vote change this to be an enum value or string representation of an enum value. That way the type and options are more well-defined. Even expecting a string and type hinting with a literal would work well.
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.
I switched to the Enum. Passing the string value is also fine. Let me know if you have other suggestions or better naming conventions.
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.
Looks fine to me, let me know when you are happy to have it merged.
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.
If it is fine, you can proceed to merge. Thanks!
This looks great! Posted one comment, but I think this is a nice addition. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #886 +/- ##
===========================================
+ Coverage 78.91% 91.20% +12.29%
===========================================
Files 75 138 +63
Lines 4217 12797 +8580
===========================================
+ Hits 3328 11672 +8344
- Misses 889 1125 +236 ☔ View full report in Codecov by Sentry. |
Following what discussed in #872, I have tried to implement an additional option for
store_trajectory
and to add the temperature parsed from the OSZICAR.For the temperature, the code will attempt to parse it if
store_trajectory
is not False, but there are different ways of doing this. For example the temperature could have been stored in a different location in the model, or its parsing could have been activated by a new option. I preferred to keep it as minimal as possible and see if there are specific requests for emmet or atomate2 compatibility (@utf and @mjwen).