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 regression: block_size argument not used #62

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

TomNicholas
Copy link
Member

#49 introduced a regression where the block_size argument didn't do anything.

This should really be tested somehow, but at the moment nothing changes in the output so not sure how to test it without any further changes.

@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #62 (d7750ff) into master (8a6765a) will increase coverage by 11.83%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #62       +/-   ##
===========================================
+ Coverage   84.08%   95.91%   +11.83%     
===========================================
  Files           2        2               
  Lines         245      245               
  Branches       74       74               
===========================================
+ Hits          206      235       +29     
+ Misses         34        7       -27     
+ Partials        5        3        -2     
Impacted Files Coverage Δ
xhistogram/core.py 97.31% <100.00%> (+15.59%) ⬆️

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 8a6765a...d7750ff. Read the comment docs.

@rabernat
Copy link
Contributor

Nice catch!

We have never done any systematic investigation of the effect of this parameter. I'm tempted to remove it from the user API and just hard code something. What do you think?

@TomNicholas
Copy link
Member Author

I think no-one is likely to bother changing this parameter, especially as memory usage can be limited with dask chunks now anyway. Therefore I personally think that if hard-coding a block size is good enough for numpy then it's good enough for us too. (In numpy they picked a power of 2 for the block size, but I don't really know why.)

@rabernat rabernat merged commit 38becf6 into xgcm:master Jun 21, 2021
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