-
Notifications
You must be signed in to change notification settings - Fork 30
Repack Nwb Files #1003
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?
Repack Nwb Files #1003
Conversation
src/neuroconv/tools/nwb_helpers/_configuration_models/_hdf5_dataset_io.py
Outdated
Show resolved
Hide resolved
src/neuroconv/tools/nwb_helpers/_configuration_models/_hdf5_dataset_io.py
Outdated
Show resolved
Hide resolved
@CodyCBakerPhD, the code now reflects my vision for the API: The code then progresses along 2 paths:
lmk what you think! |
...est_backend_and_dataset_configuration/test_helpers/test_get_default_backend_configuration.py
Show resolved
Hide resolved
...est_backend_and_dataset_configuration/test_helpers/test_get_default_backend_configuration.py
Outdated
Show resolved
Hide resolved
...est_backend_and_dataset_configuration/test_helpers/test_get_default_backend_configuration.py
Outdated
Show resolved
Hide resolved
data_chunk_iterator_kwargs = dict() | ||
else: | ||
data_chunk_iterator_class = DataChunkIterator | ||
data_chunk_iterator_kwargs = dict(buffer_size=np.prod(dataset_configuration.buffer_shape)) |
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.
use math and not np.prod as that will overflow in most cases and screw up your buffer case.
Are these changes related to repacking?
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.
use math and not np.prod as that will overflow in most cases and screw up your buffer case.
Ok, will do.
Are these changes related to repacking?
Yes, the DataChunkIterator solves this issue: hdmf-dev/hdmf#1170
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.
Ah, thanks, OK. so hdmf does not support re-setting the chunking without copying so we do this through an iterator.
Mmm, you are solving your problem and this is great but in this PR as well as the one that you did for detecting compound data types I kind of feel that hdmf is kicking its complexity up to us for something that it should provide.
I guess that we are faster : P
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.
Speaking of which, I ran into another issue with hdmf-zarr when it comes to compound dtypes -- see here: hdmf-dev/hdmf-zarr#272
Done! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1003 +/- ##
==========================================
+ Coverage 90.31% 90.61% +0.30%
==========================================
Files 138 138
Lines 8876 9000 +124
==========================================
+ Hits 8016 8155 +139
+ Misses 860 845 -15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Fixes #892
Depends on