-
Notifications
You must be signed in to change notification settings - Fork 68
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
[Feature Request]: Better handling of parsed trajectory in VASP calculations #872
Comments
The Trajectory object itself also has a very large file size compared to other file formats which store trajectories (anecdotal), I think maybe 3-5x. I wonder if it could be store more compactly, since the benefit of storing with JSON isn’t really realised since individual keys within the Trajectory are rarely used for searching etc. |
@gpetretto thanks for your post on this. I have no objections on the pure emmet side. Happy push through whatever the atomate2 folks agree with. |
Thanks @gpetretto! I've seen your implementation in issue #886 and I agree with your proposed approach. Definitely, we need to reduce the redundancy. |
@mkhorton Interesting observation! Would be great if we could further reduce the storage need. Can we easily serialize a MSONable object into other formats like hdf5? Or, by |
Problem
The current calculation model used for MD simulations in atomate2 relies on the
store_trajectory
option in theCalculation
model.I think that there are some problems associated with the current handling of the trajectory. Those related to emmet concern the amount of data that is stored. In fact, for each frame of the
Trajectory
object the wholeIonicStep
is stored as wellemmet/emmet-core/emmet/core/vasp/calculation.py
Lines 732 to 734 in ebbe8d5
Considering that this feature is used to store the output of (potentially long) MD simulation, this has some issues:
Structure
of the step, so each Structure is essentially stored twice in the Trajectory. It seems that the structure stored in the IonicStep does not include any additional property, so it is entirely redundant.The first two points are not blocking issues, but when storing the output of long simulations they will probably have a big impact on the size required for the stored data. Combined, they will probably require roughly 3x the space required if only the structure and the final properties would be stored for each step.
The parsing of large MD trajectory may also be demanding in terms of resources and in general I believe that this could be optionally handled by extracting the data from the new vasp hd5 output file, rather than from the vasprun.xml. So this issue might be linked to the one I opened in atomate2: materialsproject/atomate2#515
Also tagging @utf and @mjwen as this may impact the MD flow in atomate2.
Proposed Solution
I am aware of the fact that if the trajectory is stored its content covers also the one of the
ionic_steps
in theCalculationOutput
model. But I think that it would be reasonable to have the following changesstructure
attribute is even optional inIonicStep
IonicStep
(except for the Structure) or a subset of the data.To preserve backward compatibility this could be defined as
partial
value removes the electronic steps from the dictionary. I would make this the default for the atomate2 MD worflows.Concerning the temperature, this could be retrieved from the OSZICAR and is already available in the
Oszicar
object in pymatgen. This would require the parsing of an additional object, but, as far as I can see, the values are not present in the vasprun.xml.I am not sure if this would better fit in the
frame_properties
of the trajectory, or as an attribute of the model on its own.If these suggestions are fine I can open a PR with their implementation. Otherwise, do you have other suggestions on how to modify the current model?
Alternatives
No response
The text was updated successfully, but these errors were encountered: