Skip to content

Conversation

pauladkisson
Copy link
Contributor

@pauladkisson pauladkisson commented Jul 24, 2025

Fixes #1296

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Does the PR clearly describe the problem and the solution?
  • Have you reviewed our Contributing Guide?
  • Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

@rly
Copy link
Contributor

rly commented Jul 26, 2025

The approach is generally right but

  1. We should update HDMF Zarr to add ZarrDataIO.get_io_params to be parallel with H5DataIO and copy the link_data setting. I opened a PR Add ZarrDataIO.get_io_params hdmf-zarr#282
  2. Let's not add hdmf_zarr as a dependency. It creates circular dependency challenges and we are not quite ready to add zarr as a dependency for everyone who is not using zarr. This is a place where we have some a poor job separating the backend from the mapper. Let me think on this but I think we should test whether hdmf_zarr is installed at the top of the module, store that as a Boolean, and add that boolean in this check. So the HDMF zarr code is only run if HDMF zarr is installed (which should be the case if ZarrDataIO is used.) You can see a similar pattern elsewhere in HDMF

Copy link

codecov bot commented Jul 28, 2025

Codecov Report

❌ Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.61%. Comparing base (886bf4a) to head (1480c7b).

Files with missing lines Patch % Lines
src/hdmf/build/objectmapper.py 57.14% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1297      +/-   ##
==========================================
- Coverage   91.62%   91.61%   -0.02%     
==========================================
  Files          42       42              
  Lines        9684     9691       +7     
  Branches     1963     1964       +1     
==========================================
+ Hits         8873     8878       +5     
- Misses        523      525       +2     
  Partials      288      288              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pauladkisson
Copy link
Contributor Author

  1. We should update HDMF Zarr to add ZarrDataIO.get_io_params to be parallel with H5DataIO and copy the link_data setting. I opened a PR Add ZarrDataIO.get_io_params hdmf-zarr#282

Done.

  1. Let's not add hdmf_zarr as a dependency. It creates circular dependency challenges and we are not quite ready to add zarr as a dependency for everyone who is not using zarr. This is a place where we have some a poor job separating the backend from the mapper. Let me think on this but I think we should test whether hdmf_zarr is installed at the top of the module, store that as a Boolean, and add that boolean in this check. So the HDMF zarr code is only run if HDMF zarr is installed (which should be the case if ZarrDataIO is used.) You can see a similar pattern elsewhere in HDMF

And done!

@pauladkisson
Copy link
Contributor Author

Looks like I need to add a test for these changes, but I'm not sure how to integrate it with the existing test structure. @rly can you point me in the right direction?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: compound Dtypes do not support custom chunking and compression with Zarr.

2 participants