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

Wrap data in set_data_io with a DataChunkIterator to support overriding hdf5 dataset backend configurations #1172

Merged
merged 19 commits into from
Sep 16, 2024

Conversation

pauladkisson
Copy link
Contributor

@pauladkisson pauladkisson commented Aug 15, 2024

Motivation

Fixes #1170

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?

@pauladkisson
Copy link
Contributor Author

@oruebel, are there any problems with wrapping all data in a DataChunkIterator (numpy arrays, lists, tuples)? Should this fix be restricted to hdf5 datasets?

@oruebel
Copy link
Contributor

oruebel commented Aug 15, 2024

are there any problems with wrapping all data in a DataChunkIterator (numpy arrays, lists, tuples)? Should this fix be restricted to hdf5 datasets?

Since the user has explicit control, I think this is fine. Also,, I believe it should work with numpy, lists, and tuples as well.

@pauladkisson pauladkisson marked this pull request as ready for review August 15, 2024 20:49
@pauladkisson pauladkisson marked this pull request as draft August 15, 2024 20:52
@pauladkisson pauladkisson marked this pull request as ready for review August 15, 2024 22:52
src/hdmf/container.py Outdated Show resolved Hide resolved
src/hdmf/container.py Outdated Show resolved Hide resolved
src/hdmf/container.py Outdated Show resolved Hide resolved
@pauladkisson
Copy link
Contributor Author

@oruebel, can you enable tests for this PR?

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.02%. Comparing base (874db31) to head (96ba306).
Report is 1 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1172   +/-   ##
=======================================
  Coverage   89.01%   89.02%           
=======================================
  Files          45       45           
  Lines        9872     9879    +7     
  Branches     2810     2812    +2     
=======================================
+ Hits         8788     8795    +7     
  Misses        767      767           
  Partials      317      317           

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

@pauladkisson
Copy link
Contributor Author

@oruebel @rly CI tests pls?

@pauladkisson
Copy link
Contributor Author

Looks like this is passing all the tests, can we merge?

@pauladkisson
Copy link
Contributor Author

@rly @oruebel

@rly
Copy link
Contributor

rly commented Sep 16, 2024

This looks good to me.

rly
rly previously approved these changes Sep 16, 2024
@rly rly enabled auto-merge (squash) September 16, 2024 22:04
@rly
Copy link
Contributor

rly commented Sep 16, 2024

Thank you @pauladkisson !

@rly rly merged commit aed1de3 into hdmf-dev:dev Sep 16, 2024
26 of 28 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.

[Feature]: Add support for overriding backend configuration in HDF5 datasets
3 participants