Preserve h5py.Dataset filter settings on export

hdmf-dev / hdmf-zarr

Zarr I/O backend for HDMF

https://hdmf-zarr.readthedocs.io/

Other

7 stars 7 forks source link

Preserve h5py.Dataset filter settings on export #153

Closed oruebel closed 8 months ago

oruebel commented 8 months ago

Motivation

Fix #152 Preserve h5py.Dataset filter settings on export

How to test the behavior?

[x] Add unit tests
[X] Add functions to ZarrDataIO to wrap h5py.Dataset and infer filter settings
[X] Update ZarrIO to wrap h5py.Dataset on write to preserve filter settings

Checklist

[x] Did you update CHANGELOG.md with your changes?
[X] Have you checked our Contributing document?
[X] Have you ensured the PR clearly describes the problem and the solution?
[X] Is your contribution compliant with our coding style? This can be checked running ruff from the source directory.
[X] Have you checked to ensure that there aren't other open Pull Requests for the same change?
[X] Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (9692a98) 84.51% compared to head (4bdb47a) 85.07%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## dev #153 +/- ## ========================================== + Coverage 84.51% 85.07% +0.56% ========================================== Files 5 5 Lines 1104 1146 +42 Branches 296 311 +15 ========================================== + Hits 933 975 +42 Misses 115 115 Partials 56 56 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

oruebel commented 8 months ago

@sinha-r @rly this PR is related to the cloud project to allow conversion from HDF5 to Zarr while preserving chunking and compression settings if possible

oruebel commented 8 months ago

@rly @mavaylon1 can you please review

rly commented 8 months ago

The code looks good to me. There are a bunch of branches in the code that translates the compression. I know it's tedious, but could you write tests for those branches? That would help make sure those lines work as intended and prevent our code coverage from dropping 1% to 83.4%. We might want to include hdmf-zarr in our core packages for the U24 at some point.

oruebel commented 8 months ago

There are a bunch of branches in the code that translates the compression. I know it's tedious, but could you write tests for those branches?

The main reason I didn't add tests for all the branches is because in order to cover those cases I believe we need to install custom HDF5 compressors, i.e., add hdf5_plugins as a dependency just to run tests and szip may not be possible to test because it requires a license. In principle, we just need a HDF5 dataset that looks like it uses those filters, but I'm not sure if we can "fake" that easily.

rly commented 8 months ago

add hdf5_plugins as a dependency just to run tests

I think that is OK. It would be added to https://github.com/hdmf-dev/hdmf-zarr/blob/dev/requirements-dev.txt

szip may not be possible to test because it requires a license

We can just exclude that one from coverage

oruebel commented 8 months ago

@rly Can you please take another look. I added a new unit test module for ZarrDataIO. As mentioned above, I had to add hdf5plugin as a requirement for these tests. I added @unittest.skipIf to the test cases that use hdf5plugin just in case the library is not installed. The tests now cover 100% of the patch.

rly commented 8 months ago

Thanks! Looks good to me.