-
Notifications
You must be signed in to change notification settings - Fork 2
Implement writing to same h5md files on multiple steps #39
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?
Implement writing to same h5md files on multiple steps #39
Conversation
|
I tested my implementation in a simulation and found that when saving to H5MD file every step over 10 steps, the simulation time doubles. Since I would like to save only every 1000 steps, that would yield a performance comparable to 1001 steps, which sounds very tolerable to me. I don't know if my implementation might run into memory problems eventually, though, due to the file being open all the time and its content growing from step to step. I'll conduct further tests. |
XzzX
left a comment
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.
Ok, I would need more time to really dig into it. So just a few high level comments.
- How does
dumpanddumpStepinteract? What happens if I call both interleaving? If they do not share any logic, does it make sense to separate it into two classes? - As you experienced, there is a lot of typing and almost code duplication involved. To tackle this problem I used to generate these files via python and Jinja templates. This is not a must, but currently your changes would be overwritten.
- Can we use RAII for open and close? Aka, currently you might forget to call close. This can be avoided by binding open to the constructor of the class and close to the destructor. Similar to python
with open(...) as f:.
As of now, there is no interaction in between the two. We could separate them. The alternative would be merging. In principle, the dump function also opens, dumps and closes, so it could be changed to contain only calls to the corresponding functions. |
I see. I have no experience with Jinja, so I would have to read into it. Would it make sense to pursue implementing everything in the c++ files and as a last step incorporating the changes into the Jinja files? |
That sounds very neat! I will have a look. |
|
There seems to be an error: The positions, forces and velocities printed each step to one total file (dumpStep) are not necessarily the same as these quantities printed to one file per step (dump). I'm investigating this, but for now this branch is unstable. |
Apparently, this issue was due to something else. Anyways, I enabled reading one single requested step from an H5MD file containing an entire trajectory. Following this, I also implemented one test for the H5MD io for multiple steps and one test checking consistency between the single-step and multi-step output. I think now the changes on this PR are functionally stable. Next step will be to clean up the implementation and adjust the jinja template files. |
b776088 to
e943fa0
Compare
|
Cabana appears to have io functionality for hdf5 files as well. Might be worthwhile to check it out. |
Solves #38
Needs major redesign before being merged with main.