Skip to content
This repository has been archived by the owner on Jan 27, 2023. It is now read-only.

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

Merged
merged 1 commit into from
Aug 6, 2019

Conversation

lmohrmann
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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

@jpivarski jpivarski merged commit 2e7c280 into scikit-hep:master Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants