scikit-hep / uproot3-methods

Pythonic behaviors for non-I/O related ROOT classes.
BSD 3-Clause "New" or "Revised" License
21 stars 28 forks source link

Fix resolving TH2D from numpy.histogram2d or numpy.histogramdd #60

Closed lmohrmann closed 5 years ago

lmohrmann commented 5 years ago

This PR fixes resolving a TH2D from tuples created by numpy.histogram2d or numpy.histogramdd, which did not work for me.

There does not seem to be a test for this. Unfortunately, I don't have time to write one now, and there's already a separate issue for this: #15.

I've tested this with numpy 1.16.4 and uproot_methods 0.7.0.

jpivarski commented 5 years ago

I've looked carefully at the changes, and I see that they are corrections for both numpy.histogram2d and numpy.histogramdd. Only one character has changed in each:

# for numpy.histogram2d:
obj[0].shape[1] == obj[1].shape[0] - 1      # original
obj[0].shape[1] == obj[2].shape[0] - 1      # correction
#                      ^

# for numpy.histogramdd:
obj[0].shape[0] == obj[1][1].shape[0] - 1   # original
obj[0].shape[1] == obj[1][1].shape[0] - 1   # correction
#            ^

Both of these are verifying that the second dimension of the 2D counts array has one fewer than the number of second-dimension edges (fencepost principle). The mistake for numpy.histogram2d was that I didn't choose the second edge array and the mistake for numpy.histogramdd was that I didn't choose the second dimension of the 2D counts array. They're different mistakes, but both evidence of uncareful copy-paste.

In both cases, this might have slipped by if the number of X bins is equal to the number of Y bins in a test case. Does your test case have a different number of X and Y bins? Because if so, then it's fully tested—this won't regress any cases because there can't be any valid 2D histograms that don't have these exact dimensions.

Thanks!

jpivarski commented 5 years ago

Oh, and please respond @lmohrmann, I'll merge this as soon as you verify that your test case had a different number of X bins than Y bins. That would be enough for me.

lmohrmann commented 5 years ago

Hi @jpivarski, yes, indeed, my histogram had different numbers of x and y bins! Thanks for looking into this!