ParticleGroup.from_hdf5 - #156
Conversation
ken-lauer
left a comment
ken-lauer
left a comment
There was a problem hiding this comment.
I think this looks good in general
| # particle_paths returns absolute paths. Strip the leading separator so that | ||
| # they resolve relative to h5, which may itself be a group within a file. | ||
| path = paths[0].strip("/") | ||
| return h5[path] if path else h5["."] |
There was a problem hiding this comment.
Is h5["."] just to consistently return a Group instead of the File?
There was a problem hiding this comment.
Yes, exactly.
There was a problem hiding this comment.
But File is a subclass of Group?
(I don't see an issue with doing this, really, it's just a bit of a curiosity with respect to covariance/contrainvariance...)
There was a problem hiding this comment.
h5[""] also raises if you strip the path and it was /.
| logger.debug("Loading iteration %s from %s", paths[0], h5.name) | ||
|
|
||
| # particle_paths returns absolute paths. Strip the leading separator so that | ||
| # they resolve relative to h5, which may itself be a group within a file. |
There was a problem hiding this comment.
Is this a thing in the standard? I always assumed attrs/etc had to be at the top level
There was a problem hiding this comment.
It is not a part of the standard. However, I know multiple people we work with that use this as a feature.
There was a problem hiding this comment.
Is this one of those upstream issues you had made by chance?
There was a problem hiding this comment.
Actually, no, I can add it, however.
There was a problem hiding this comment.
Looking more into it, the standard already has a feature to allow embedding with basePath and I think asking them to allow for a second method of doing this defeats the point of a standard.
Since I know some people do this, but it's non-standards-compliant behavior and from Chris's comment earlier today, I'm going to add a warning here that this is non standards compliant, but keep it here to avoid breaking code immediately.
| return value[0] | ||
|
|
||
|
|
||
| def _only_iteration_group(h5: File | Group) -> Group: |
There was a problem hiding this comment.
It feels like these specialized "load just one" helpers should be a wrapper around a more general one that walks - i.e., yields as a generator - (h5_group, iteration, species) in a given file.
I think that can wait until a future refactor.
There was a problem hiding this comment.
Yeah, this feels like a larger project of adding support for iterations and species - complicated, from personal experience. Let's revisit later.
Co-authored-by: Ken Lauer <152229072+ken-lauer@users.noreply.github.com>
|
I also added a warning to |
3 participants
Per discussion with @ChristopherMayes, this PR adds a new method
.from_hdf5toParticleGroupwith atime_offsetargument. Additional cleanup is performed.pre-committo conda env.vscodeto.gitignore__init__argumenth5h5py.File,h5py.Group, "legacy" group with no speciesexceptions.pywith custom BeamPhysics exceptions for HDF5 file load errorsload_bunch_dataand__init__are broken into methods to handle each atomic part of loading the only species in the only iteration of a particle group.load_species_datagets the array dict from one species_only_species_grouploads the only species in the OpenPMD iteration passed to it (or passes through to handle "legacy" version w/o species)_only_iteration_groupgets the only iteration, raising on none or multiple. Handles files, groups, paths._only_iteration_only_species_grouphandles the file loading, existing behavior of passing through raw species group.load_bunch_datatoreaders.pyand keep import there for legacy supportload_time_offsetto load from an open species groupload_only_time_offsetloads from only species from only iteration with same input semantics as.from_hdf5t_offsettoParticleGroupt.writedocs/examples/write_examples.ipynbupdated to use read w/o time offsetdocs/examples/read_examples.ipynbdemostrates.from_hdf5and time offset methodsdocs/api/readers.mdopenPMDattribute in root group of OpenPMD files. InParticleGroup.__init__warn if not valid (to preserve legacy behavior), infrom_hdf5raise.Note: this PR also includes the changes noted in #152. It was intended as a stacked PR, but due to working out of a fork is set up to target main instead and will supersede it.