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

Fix Y binning and revert TH2.numpy behavior #32

Merged
merged 1 commit into from
Jan 7, 2019

Conversation

nsmith-
Copy link
Contributor

@nsmith- nsmith- commented Jan 6, 2019

Fixes #31
Changes to return type of TH2.numpy should come with a minor version change

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix to the attribute name (surprising that the Axis class uses the letter "x" in its names), but the change to the output of the numpy property was deliberate. It's for similarity with the numpy.histogram2d function, which returns a flat, 3-item tuple. Do you disagree that this should be a goal? (I'm open to counter-arguments; the above is my reasoning.)

@lgray
Copy link
Collaborator

lgray commented Jan 6, 2019

Either format is fine to me. So long as it's well advertised. :-)

@nsmith-
Copy link
Contributor Author

nsmith- commented Jan 7, 2019

@jpivarski to me, nested edges generalizes to n-D better, but since the method is called numpy it should probably behave like numpy. One can always get edges and values separately. What about whether to include overflow?

@jpivarski
Copy link
Member

These methods map onto ROOT histograms, which only go up to 3D. However, even if generalization was an issue, I don't see how unpacking flat vs. nested is different for an unknown number of dimensions:

values, edges = hist.numpy()

Gives you edges as an array for 1D and a 2- or 3-tuple of arrays for 2D or 3D, regardless of whether the result was flat or nested. I assume this is how you'd want to unpack it. If you know the dimensionality, you'd say

values, xedges, yedges = hist.numpy()

for flat and

values, (xedges, yedges) = hist.numpy()

for nested. So it's only if you do know the number of dimensions that there's a difference.

It wouldn't be like Numpy to include overflow bins, but following the other conventions, that could be a hist.allnumpy() method.

@nsmith-
Copy link
Contributor Author

nsmith- commented Jan 7, 2019

Works for me, done

@jpivarski jpivarski merged commit 4f6edc4 into scikit-hep:master Jan 7, 2019
jpivarski added a commit that referenced this pull request Jan 7, 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.

3 participants