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: input arrays are not broadcast #61

Closed gjoseph92 closed 3 years ago

gjoseph92 commented 3 years ago

I noticed that #49 introduced a regression where input arrays now all must be the same shape (before, they'd be broadcast when possible). In https://github.com/xgcm/xhistogram/pull/49#discussion_r640872588 I was trying to explain this and a potential fix, but probably wasn't very clear about it.

Here's a test demonstrating the issue; this passes on HEAD^ (as long as dask is installed from main). I'll push up the rest of the fix in a few hours.

cc @TomNicholas @rabernat

rabernat commented 3 years ago

Thanks so much for this @gjoseph92! Sorry we missed your suggestion. #49 got a bit hard to follow by the end.

codecov[bot] commented 3 years ago

Codecov Report

Merging #61 (b20c3f9) into master (d1df004) will decrease coverage by 0.06%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
- Coverage   84.14%   84.08%   -0.07%     
==========================================
  Files           2        2              
  Lines         246      245       -1     
  Branches       77       74       -3     
==========================================
- Hits          207      206       -1     
  Misses         34       34              
  Partials        5        5              
Impacted Files Coverage Δ
xhistogram/core.py 81.72% <100.00%> (-0.10%) :arrow_down:

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 fe1b3fa...b20c3f9. Read the comment docs.

TomNicholas commented 3 years ago

Thanks @gjoseph92 for catching this!

The PR looks good to me, but there some test failures on some environments (all with python 3.7 it seems) of a form where all_arrays = broadcast_arrays(*all_arrays) is somehow causing

    def raise_if_computed():
>       raise ValueError("Triggered forbidden computation")
E       ValueError: Triggered forbidden computation

xhistogram\test\fixtures.py:8: ValueError
TomNicholas commented 3 years ago

there are some test failures

Attempting to reproduce locally (on Ubuntu) suggests that a more recent numpy version will pass on python 3.7, so perhaps any future release of this functionality should just bump the dependencies a bit?

gjoseph92 commented 3 years ago

Thanks for looking into that. Yes, this will require NumPy >= 0.17 so that NEP-18 is active by default. I've updated that in this PR. Incidentally I saw that the NumPy version used in CI is different for all three Python versions; is that intentional?

jrbourbeau commented 3 years ago

I saw that the NumPy version used in CI is different for all three Python versions; is that intentional?

Yes, that's so we can provide some CI coverage over multiple versions of NumPy (we do something similar in dask/dask)