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.

Sign up for GitHub

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

Jump to bottom

pass through DataIO params after formatting strings #173

Merged
merged 7 commits into from
Oct 16, 2019
Merged

pass through DataIO params after formatting strings #173

merged 7 commits into from
Oct 16, 2019

Conversation

ajtritt
Copy link
Contributor

@ajtritt ajtritt commented Oct 16, 2019

Motivation

fix #162

@ajtritt ajtritt requested review from rly, bendichter and oruebel October 16, 2019 19:10
@codecov
Copy link

codecov bot commented Oct 16, 2019 •
edited
Loading

Codecov Report

Merging #173 into dev will increase coverage by 0.29%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev    #173      +/-   ##
=========================================
+ Coverage   70.01%   70.3%   +0.29%     
=========================================
  Files          30      30              
  Lines        5813    5823      +10     
  Branches     1369    1370       +1     
=========================================
+ Hits         4070    4094      +24     
+ Misses       1307    1298       -9     
+ Partials      436     431       -5
Impacted Files Coverage Δ
src/hdmf/build/map.py 67.59% <100%> (+1.46%) ⬆️
src/hdmf/backends/hdf5/h5_utils.py 63.9% <100%> (+0.71%) ⬆️
src/hdmf/data_utils.py 89.93% <66.66%> (-0.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0022114...92f18b4. Read the comment docs.

bendichter
bendichter reviewed Oct 16, 2019
View reviewed changes
Copy link
Contributor

@bendichter bendichter left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It would be nice to see a full IO-to-disk test that shows the resulting HDF dataset is in fact chunked

src/hdmf/data_utils.py
"""
Returns a dict with the I/O parameters specifiedin in this DataIO.
"""
return dict()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right? Just return an empty dict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base DataIO class offers no ability to specify parameters, so there's not really anything to return

oruebel
oruebel previously approved these changes Oct 16, 2019
View reviewed changes
Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for chasing down this bug.

@rly
Copy link
Contributor

rly commented Oct 16, 2019

It would be nice to see a full IO-to-disk test that shows the resulting HDF dataset is in fact chunked

Agreed

@ajtritt ajtritt merged commit a115dc6 into dev Oct 16, 2019
@rly rly deleted the bug/chunked_strings branch January 7, 2020 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bendichter bendichter bendichter left review comments

@rly rly rly approved these changes

@oruebel oruebel oruebel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

H5DataIO.chunks arg ignored for strings
4 participants
@ajtritt @rly @bendichter @oruebel