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

Avoid reading first chunk of DataChunkIterator on init if maxshape and dtype are specified #189

Merged
merged 3 commits into from
Oct 28, 2019

Conversation

rly
Copy link
Contributor

@rly rly commented Oct 28, 2019

If a DataChunkIterator is constructed with an iterator for data, then the first chunk is read on init in order to help determine the maxshape and dtype. This is unnecessary if both maxshape and dtype are provided to init, so we should avoid reading the first chunk. This helps with #136.

It looked like someone had already written in this condition years ago but left it commented out. This PR uncomments that condition.

@rly rly requested review from oruebel and ajtritt October 28, 2019 16:59
@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #189 into dev will not change coverage.
The diff coverage is 90%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #189   +/-   ##
=======================================
  Coverage   70.32%   70.32%           
=======================================
  Files          30       30           
  Lines        5911     5911           
  Branches     1380     1381    +1     
=======================================
  Hits         4157     4157           
  Misses       1323     1323           
  Partials      431      431
Impacted Files Coverage Δ
src/hdmf/data_utils.py 90% <90%> (ø) ⬆️

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 8ddae22...07d3b0c. Read the comment docs.

ajtritt
ajtritt previously approved these changes Oct 28, 2019
Copy link
Contributor

@ajtritt ajtritt left a comment

Choose a reason for hiding this comment

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

Seems like a good idea to bring that check back. Is there a reason why we don't move setting of maxshape to the getter i.e. only attempt to read the next chunk for setting maxshape when it is read ?

@rly
Copy link
Contributor Author

rly commented Oct 28, 2019

Is there a reason why we don't move setting of maxshape to the getter i.e. only attempt to read the next chunk for setting maxshape when it is read ?

That seems like a reasonable thing to do. I'll refactor it.

@rly rly merged commit b247ad6 into dev Oct 28, 2019
@rly rly deleted the enh/dci_first_chunk branch October 28, 2019 23:46
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.

2 participants