Skip to content
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

Adding H5MDWriter to H5MD.py #2866

Closed
edisj opened this issue Jul 24, 2020 · 7 comments · Fixed by #3189
Closed

Adding H5MDWriter to H5MD.py #2866

edisj opened this issue Jul 24, 2020 · 7 comments · Fixed by #3189
Assignees
Labels
Component-Writers Format-H5MD hdf5-based H5MD trajectory format NSF REU NSF Research Experience for Undergraduates project
Milestone

Comments

@edisj
Copy link
Contributor

edisj commented Jul 24, 2020

Is your feature request related to a problem?

Currently in my pull request (PR#2787), we've only implemented an H5MD file reader. I'd like to implement a writer as well using H5PY's libraries for writing HDF5 files. H5MD files follow specific rules for the structure of groups and datasets. There's already a package, pyh5md, that can write H5MD files nicely, but it's no longer being maintained by its author, so we've decided to stick to H5PY for writing the files.

Describe the solution you'd like

We've been given permission from Pierre de Buyl, the author of pyh5md, to borrow code from his library and implement it into H5MDWriter. I've already begun a draft that's mostly working (uploaded to my github here), where I modeled it after NCDFWriter and added/tweaked code from pyh5md. I would like to open a pull request as soon as the reader is merged.

Describe alternatives you've considered

We could write the writer from scratch without using pyh5md, but that sounds a lot harder.

Additional context

H5MD format
NCDFWriter - the writer I used as a template to write mine
pyh5md code - used TimeElement class, create_box(), default_chunks(), and element()

@orbeckst orbeckst added Format-H5MD hdf5-based H5MD trajectory format Component-Writers labels Jul 24, 2020
@orbeckst
Copy link
Member

  • make a PR (or does it require add h5md format #2787 to be merged first?)
  • When we use pyh5md code then we
    • add it as separate commit and set the author to @pdebuyl --author="Pierre de Buyl <...>
    • add pyh5md license information to the LICENSE file

@edisj
Copy link
Contributor Author

edisj commented Jul 24, 2020

It doesn't have to be merged first, I guess. I just thought it would be awkward to keep pushing commits to both PRs from the same file. Is there a more convenient way to handling that than, say, deleting H5MDWriter, pushing to reader PR, then deleting H5MDReader and pushing to the writer PR?

edit: nevermind, I think this is all handled by creating a new branch

@orbeckst
Copy link
Member

Yes, indeed, branches!

@edisj edisj mentioned this issue Jul 24, 2020
4 tasks
@orbeckst
Copy link
Member

FYI: discussion on empyty boxes #1738

@pdebuyl
Copy link

pdebuyl commented Jul 29, 2020

H5MD allows "boundary" (a length D vector attribute) to be none.

https://nongnu.org/h5md/h5md.html#simulation-box

@orbeckst orbeckst added the NSF REU NSF Research Experience for Undergraduates project label Aug 7, 2020
@orbeckst
Copy link
Member

@edisj is your writer PR out of draft stage?

@edisj
Copy link
Contributor Author

edisj commented Mar 16, 2021

@edisj is your writer PR out of draft stage?

yes - I've just pushed a working cleaned up version to the pull request #2869

@orbeckst orbeckst added this to the 2.1.0 milestone May 5, 2021
IAlibay pushed a commit that referenced this issue Aug 21, 2021
Fixes #2866

## Work done in this PR
- Adds H5MDWriter
- Adds entry on ts.data to Timestep documentation
- Adds duecredit entries to H5MD reader & writer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component-Writers Format-H5MD hdf5-based H5MD trajectory format NSF REU NSF Research Experience for Undergraduates project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants