xgcm / xhistogram

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

Add density=True option #17

Closed TomNicholas closed 4 years ago

TomNicholas commented 4 years ago

I just did it in the simplest way that @rabernat described.

I left the N-D histogram case for later, and I'm also not sure what would happen if you passed density=True for multiple DataArray arguments to xhistogram.xarray.histogram.

Someone should double-check the logic because I'm not 100% confident I handled the counting of all the bins correctly.

Also small numerical differences (~10^-17) meant I had to use np.testing.assert_allclose instead of np.testing.assert_arrays_equal, I assume that's okay?

codecov-io commented 4 years ago

Codecov Report

Merging #17 into master will increase coverage by 0.40%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
+ Coverage   93.65%   94.06%   +0.40%     
==========================================
  Files           3        3              
  Lines         205      219      +14     
  Branches       54       60       +6     
==========================================
+ Hits          192      206      +14     
  Misses          8        8              
  Partials        5        5              
Impacted Files Coverage Δ
xhistogram/core.py 98.48% <100.00%> (+0.13%) :arrow_up:
xhistogram/xarray.py 91.07% <100.00%> (+0.50%) :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 c53fea6...0a6e761. Read the comment docs.

rabernat commented 4 years ago

I don't know how I let this sit for so long. Thanks so much @TomNicholas for your contribution!

rabernat commented 4 years ago

Thanks @dougiesquire for your comments here! @TomNicholas, let us know how we can help you move forward with this PR.

One option is that we could make @dougiesquire a maintainer on xhistogram and let him push directly to your branch with his suggestion above.

TomNicholas commented 4 years ago

Hi both - I had forgotten about this but thanks for your reviews. I've just added @dougiesquire 's suggestions, which seem to work nicely. I'm not sure how else to explain how the normalisation works apart from a dedicated example in the docs, but perhaps that could be left for another PR?

rabernat commented 4 years ago

This is great, thanks @TomNicholas. I think we can move forward as is. Unfortunately travis notifications have broken on this repo, but your tests ran and pass. So I will merge now.

bradyrx commented 4 years ago

Thanks everyone!

chiaral commented 3 years ago

Hello - just a comment to say that this seems to be unreleased - last release was in August. Whenever there is time, please release it because it's a great feature :)

rabernat commented 3 years ago

Thanks for the ping! I just release 0.1.2.