xgcm / xhistogram

Fast, flexible, label-aware histograms for numpy and xarray
https://xhistogram.readthedocs.io
MIT License
89 stars 19 forks source link

Fix regression: block_size argument not used #62

Closed TomNicholas closed 3 years ago

TomNicholas commented 3 years ago

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[bot] commented 3 years ago

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%) :arrow_up:

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 commented 3 years ago

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 commented 3 years ago

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.)