hdmf-dev / hdmf-zarr

Zarr I/O backend for HDMF
https://hdmf-zarr.readthedocs.io/
Other
7 stars 7 forks source link

Rework ZarrIO backend/Remove python3.7/Update HDMF and PyNWB min/Update workflows #120

Closed mavaylon1 closed 11 months ago

mavaylon1 commented 11 months ago

Motivation

What was the reasoning behind this change? Please explain the changes briefly. This branch holds the re-work of the ZarrIO backend on how it handles object references. The changes are to follow the same workflow HDMF has when handling object references.

With the changes to the backend, we have a temporary ignore on dtype conversion. It is has been raised in this ticket: https://github.com/hdmf-dev/hdmf-zarr/pull/120

This PR removes python 3.7, brings hdmf and pynwb to the most up to date, as well updates to the workflows.

How to test the behavior?

Show how to reproduce the new behavior (can be a bug fix or a new feature)

Run unit tests and gallery examples, specifically test_io_convert.py and plot_convert_nwb_hdf5.py

Checklist

mavaylon1 commented 11 months ago

Fix #121 Fix #113 Fix #106 Fix #79

codecov-commenter commented 11 months ago

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (6c13e14) 81.73% compared to head (f3d1abe) 84.73%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #120 +/- ## ========================================== + Coverage 81.73% 84.73% +3.00% ========================================== Files 11 12 +1 Lines 2667 2903 +236 ========================================== + Hits 2180 2460 +280 + Misses 487 443 -44 ``` | [Files](https://app.codecov.io/gh/hdmf-dev/hdmf-zarr/pull/120?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hdmf-dev) | Coverage Δ | | |---|---|---| | [tests/unit/base\_tests\_zarrio.py](https://app.codecov.io/gh/hdmf-dev/hdmf-zarr/pull/120?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hdmf-dev#diff-dGVzdHMvdW5pdC9iYXNlX3Rlc3RzX3phcnJpby5weQ==) | `98.50% <100.00%> (ø)` | | | [tests/unit/utils.py](https://app.codecov.io/gh/hdmf-dev/hdmf-zarr/pull/120?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hdmf-dev#diff-dGVzdHMvdW5pdC91dGlscy5weQ==) | `77.91% <100.00%> (+14.41%)` | :arrow_up: | | [tests/unit/test\_io\_convert.py](https://app.codecov.io/gh/hdmf-dev/hdmf-zarr/pull/120?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hdmf-dev#diff-dGVzdHMvdW5pdC90ZXN0X2lvX2NvbnZlcnQucHk=) | `98.65% <99.28%> (+0.78%)` | :arrow_up: | | [src/hdmf\_zarr/backend.py](https://app.codecov.io/gh/hdmf-dev/hdmf-zarr/pull/120?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hdmf-dev#diff-c3JjL2hkbWZfemFyci9iYWNrZW5kLnB5) | `90.33% <91.30%> (+2.45%)` | :arrow_up: | | [src/hdmf\_zarr/zarr\_utils.py](https://app.codecov.io/gh/hdmf-dev/hdmf-zarr/pull/120?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hdmf-dev#diff-c3JjL2hkbWZfemFyci96YXJyX3V0aWxzLnB5) | `79.28% <79.28%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/hdmf-dev/hdmf-zarr/pull/120/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hdmf-dev)

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

mavaylon1 commented 11 months ago

@rly I did the updates for using export_source for a flag; however, we would need to flush it out more with the export updates in the future. (specifically in links). The scope of these this PR is getting to large.

oruebel commented 11 months ago

We should also update the note here https://github.com/hdmf-dev/hdmf-zarr/blob/dev/docs/source/storage.rst#region-references to mention that the new resolver classes will also need to be updated to implement region references

mavaylon1 commented 11 months ago

We should also update the note here https://github.com/hdmf-dev/hdmf-zarr/blob/dev/docs/source/storage.rst#region-references to mention that the new resolver classes will also need to be updated to implement region references

Done.

rly commented 11 months ago

This looks good to me. I'll let @oruebel do the final approval.

rly commented 11 months ago

Thanks for your hard work on this!