Add ability to store multiple cells per HDF5 #63
Conversation
Pull Request Test Coverage Report for Build 8973181701Details
💛 - Coveralls |
There was a problem hiding this comment.
The main question that I have is: do these changes require one to know the prefixes that have been saved in order to read the data?
The reason I ask is: in the "released" version, it is relatively straightforward; when I read a battdata.h5, I expect to have at least a raw_data and a metadata fields, and, in most occasions, a cycle_stats one as well. With these newer changes, it is not clear to me how I would get any data without knowing the prefix. In my understanding, the inspect_batdat can inform us which prefixes are stored, which definitely alleviates this.
| # Prepend the prefix | ||
| if prefix is not None: | ||
| key = f'{prefix}-{subset}' | ||
| else: |
There was a problem hiding this comment.
If we want to get all the cells stored in the hdf5 file, regardless of 'prefix', does this work? Or would it raise an error?
Also, doesn't this require one to know all the prefixes that have been stored?
There was a problem hiding this comment.
Yea, it does require knowing the prefixes if they are used when saving the file.
Maybe we should return an error if the prefix is not included in those found in the file?
There was a problem hiding this comment.
I'm going to add that warning message
| @@ -0,0 +1,45 @@ | |||
| # The `BatteryDataset` Object | |||
There was a problem hiding this comment.
I just want to say I think this is a fantastic idea! A code-free full description of the main class we use will make it much easier for us to use and alter it as needs arise, as well as for other users to smoothly get started. Nicely done!
There was a problem hiding this comment.
Yep, we totally need this for a bunch of features. Will gradually piece it together
| return metadata, names | ||
|
|
||
| @staticmethod | ||
| def get_metadata_from_hdf5(path: Union[str, Path]) -> BatteryMetadata: |
There was a problem hiding this comment.
I'm a little confused... Is this a different function from what it used to be?
There was a problem hiding this comment.
Do you mean different than the old method for reading the metadata from an HDF file?
There was a problem hiding this comment.
Yeah, comparing the additions and deletions on Github it appears they are the same function? Maybe just moved to a different place in the file?
There was a problem hiding this comment.
Correct. It just moved
|
Good point a out the potential annoyance of having to lookup prefixes before reading. How about I add an 'auto' option to the read which will grab data belonging to the first prefix then warn if there are more than one available? |
|
I think the 'auto' idea is good, my question there is: how is the "first" prefix identified? I mean, there's no inherent order to them, right? I also think there should be a 'all' option, which basically loads all prefixes available. If you think it's wise, the 'all' can also output a warning for memory constraints. |
|
@victorventuri , I opted to add a "load by index" option to supply the need for "any." Does this give you the features you'd need? |
Fixes #61