Allow TemplateBank/FilterBank to read/write from a named group within… - #5398
Open
ahnitz wants to merge 1 commit into
Open
Allow TemplateBank/FilterBank to read/write from a named group within…#5398ahnitz wants to merge 1 commit into
ahnitz wants to merge 1 commit into
Conversation
… a shared HDF5 file Adds group_key and file_handler parameters to TemplateBank.__init__ and TemplateBank.write_to_hdf, so a bank's parameters (and compressed waveforms) can live in a named subgroup of an HDF5 file instead of only at the file root, and so multiple banks can share one already-open file handle instead of each opening the file separately. This lets a single HDF5 file hold more than one related template bank (e.g. two banks that reference each other) without needing separate files on disk.
GarethCabournDavies
left a comment
GarethCabournDavies
left a comment
Contributor
There was a problem hiding this comment.
This looks good, there is a minor question about how this relates to the HGroup in pycbc.io.hdf. It may be that this is handled fine, and if your tests work then the proof is in that pudding.
Also if the file handler is given is filename actually needed?
| extra_args : {None, dict} | ||
| Any extra keyword arguments that were provided on initialization. | ||
| """ | ||
| def __init__(self, filename, approximant=None, parameters=None, |
Contributor
There was a problem hiding this comment.
if file_handler or file is given, should filename become an optional argument?
| # 1. Resolve Root File Object | ||
| if file_handler is not None: | ||
| # If passed a Group, get its file. If passed a File, use it. | ||
| if isinstance(file_handler, h5py.Group): |
Contributor
There was a problem hiding this comment.
I think this should be a HGroup from pycbc.io, but it may be best to check. We would also need to check whether isinstance(..., h5py.Group) would be true if it is a HGroup, as that is a subclass of h5py.Group
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a new feature to our basic bank class. It allows the bank to be located within an hdf5 file group of your choice rather than the assumed top level / root layout. This is required for #5395, but might be useful for other things if you say wanted multiple banks in a file.