Skip to content
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

Fix #1148 : Add passthrough on non-DCI H5DataIO to support its use in pynwb TimeSeries. #1149

Merged
merged 6 commits into from
Aug 28, 2024
Merged

Fix #1148 : Add passthrough on non-DCI H5DataIO to support its use in pynwb TimeSeries. #1149

merged 6 commits into from
Aug 28, 2024

Conversation

cboulay
Copy link
Contributor

@cboulay cboulay commented Jul 10, 2024

Fix #1148

Motivation

What was the reasoning behind this change? Please explain the changes briefly.

As explained in #1148 and pynwb #1929, when a pynwb.TimeSeries object is created where (1) its data is a H5DataIO object that DOES NOT wrap a DataChunkIterator, AND (2) its timestamps are provided explicitly (vs rate and starting_time), then the TimeSeries object crashes because it cannot access the maxshape attribute on its data value.

With this change, the H5DataIO has a new maxshape property which passes through the correct maxshape if known (either set during __init__ or inferred from the dataset) else None, and pynwb.TimeSeries no longer crashes.

How to test the behavior?

I added a unit test in unit/test_io_hdf5_h5tools.py

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?

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.92%. Comparing base (abb6fe5) to head (dc5cf24).
Report is 26 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1149      +/-   ##
==========================================
+ Coverage   88.91%   88.92%   +0.01%     
==========================================
  Files          45       45              
  Lines        9857     9866       +9     
  Branches     2802     2806       +4     
==========================================
+ Hits         8764     8773       +9     
  Misses        776      776              
  Partials      317      317              

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

@rly rly requested a review from stephprince August 8, 2024 20:29
Copy link
Contributor

@stephprince stephprince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cboulay, sorry for the delayed review. I added a couple of small suggestions but otherwise looks great. Thanks again for the PR!

src/hdmf/backends/hdf5/h5_utils.py Outdated Show resolved Hide resolved
tests/unit/test_io_hdf5_h5tools.py Outdated Show resolved Hide resolved
@cboulay
Copy link
Contributor Author

cboulay commented Aug 27, 2024

Sorry, this fell off the radar. I'll look at it this evening.

@cboulay
Copy link
Contributor Author

cboulay commented Aug 28, 2024

@stephprince , thank you for the suggestions. I incporporated all your suggested changes.

Copy link
Contributor

@stephprince stephprince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! Looks good to me

@stephprince stephprince merged commit d378dec into hdmf-dev:dev Aug 28, 2024
29 checks passed
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]: Non-DCI H5DataIO does not passthrough maxshape property which crashes pynwb Timeseries
2 participants