BUG: convert to plain ndarray in `np.histogramdd` by ikrommyd · Pull Request #32701 · numpy/numpy

ikrommyd · 2026-09-19T22:53:27Z

PR summary

Found as something annoying for #32577.
I do believe this is an oversight as histogram and histogram2d already convert to plain ndarrays and do not preserve subclasses and histogramdd also converts to plain ndarrays in the case it gets handed an iterable of ndarray subclasses. It's docstring also says that it should return a plain ndarray.

AI Disclosure

AI wrote the tests

Signed-off-by: Iason Krommydas <[email protected]>

ikrommyd · 2026-09-19T22:57:55Z

cc @seberg

Signed-off-by: Iason Krommydas <[email protected]>

seberg

I do wonder a bit how well this worked with some subclasses before, but maybe this is the sane thing? (doing this just for minmax feels a bit unclear.)

seberg · 2026-09-20T18:34:20Z

@@ -0,0 +1 @@
+* `np.histogramdd` now converts and returns a plain ndarray as its docstring promises in all cases. Previously it only did that when it was handled a sequence of arrays.


Well, "as it's docstring promises" is a stretch, all docstrings promise that :).

how about "as the other histogramming functions do"?

seberg · 2026-09-20T18:36:21Z

        # Sample is a sequence of 1D arrays.
        sample = np.atleast_2d(sample).T
        N, D = sample.shape
+    sample = np.asarray(sample)


Hmmm, this is awkward, so an ndarray is anything that has a shape attribute, here...

Maybe add a comment that this intentionally drops subclasses? The squence input is awkward anyway. The main reason for this I can see is because other histogram functions do it as well. It could be a regression (although histogramdd is maybe not super common).

Ping @mhvk since you care about subclasses. But astropy uses __array_function__ of course.

Yeah it is awkward and in the second branch np.atleast_2d(sample) drops the subclasses when handed an iterable of ndarray subclasses....

Random comment just in case Marten cares, but astropy doesn't feel fully consistent here either. Whether the counts will be wrapped in the subclass seems to depend on the other arguments. Don't know if there's a logic behind it but just saying. But I did check astropy and unyt, they don't rely on numpy preserving the subclass. Their overrides rewrap.

In [17]: from astropy import units as u In [18]: x = np.array([1,2,3]) * u.m In [19]: x Out[19]: <Quantity [1., 2., 3.] m> In [20]: np.histogram(x, weights=[1,2,3]) Out[20]: (<Quantity [1., 0., 0., 0., 0., 2., 0., 0., 0., 3.]>, <Quantity [1. , 1.2, 1.4, 1.6, 1.8, 2. , 2.2, 2.4, 2.6, 2.8, 3. ] m>) In [21]: np.histogram(x) Out[21]: (array([1, 0, 0, 0, 0, 1, 0, 0, 0, 1]), <Quantity [1. , 1.2, 1.4, 1.6, 1.8, 2. , 2.2, 2.4, 2.6, 2.8, 3. ] m>) In [22]: np.histogramdd(x) Out[22]: (array([1., 0., 0., 0., 0., 1., 0., 0., 0., 1.]), [<Quantity [1. , 1.2, 1.4, 1.6, 1.8, 2. , 2.2, 2.4, 2.6, 2.8, 3. ] m>]) In [23]: np.histogramdd(x, weights=[1,2,3]) Out[23]: (<Quantity [1., 0., 0., 0., 0., 2., 0., 0., 0., 3.]>, [<Quantity [1. , 1.2, 1.4, 1.6, 1.8, 2. , 2.2, 2.4, 2.6, 2.8, 3. ] m>])

The logic of astropy is that if there are no weights, the functions produce counts which should be integer arrays and thus are illogical as quantities (histogramdd appears to be inconsistent in producing float, but that is outside of astropy's control), while if there are weights, the counts get multiplied by it, so it should get the unit of the weight. The choice made is to not specifically check whether the weight itself is in fact a quantity; for float, this does not really matter, since a dimensionless quantity and float ndarray behave the same, but for integer weights arguably it is not quite right, in that one would like to get an integer array out.

ikrommyd · 2026-09-20T18:44:59Z

I do wonder a bit how well this worked with some subclasses before, but maybe this is the sane thing? (doing this just for minmax feels a bit unclear.)

My logic here was not to do this for for the sake of minmax, but because it feels inconsistent to me and like it just works by chance. It only preserves the subclass only in the single array input case of histogramdd and only if all the internal functions just happen to preserve the subclass. If you do an internal change in histogramdd and you use a function that doesn't preserve a subclass, it would stop happening. + histogram and histogram2d intentionally do asarray.
Also it doesn't preserve all subclasses, for masked arrays it doesn't preserve them for example

In [7]: x = np.ma.array([1,2,3], mask=False)

In [8]: x
Out[8]:
masked_array(data=[1, 2, 3],
             mask=[False, False, False],
       fill_value=999999)

In [9]: np.histogramdd(x)
Out[9]:
(array([1., 0., 0., 0., 0., 1., 0., 0., 0., 1.]),
 [array([1. , 1.2, 1.4, 1.6, 1.8, 2. , 2.2, 2.4, 2.6, 2.8, 3. ])])

ikrommyd · 2026-09-20T19:12:26Z

There is another argument for doing this and it has to do with masked arrays. Right now because the function uses .min() and .max() for the edges calculation, masked values are properly excluded from the edges calculation but the histogramming part will actually include them. So for this snippet:

In [9]: import numpy as np
   ...: x = np.ma.array([0., 0.5, 1., 100.], mask=[0, 1, 0, 1])
   ...: h, e = np.histogramdd(x, bins=2)
   ...: print(e[0], h, h.sum())
[0.  0.5 1. ] [1. 2.] 3.0

You get 3 values in the histogram but the edges are constructed from 2 values. All the other histogramming functions just ignore the mask, period, and there's an open issue for masked histogramming support.

Signed-off-by: Iason Krommydas <[email protected]>

mhvk

I think it is good to get the histogram functions to behave the same way, although I'd perhaps argue that it would be better to go in the other direction, preserve subclasses also for the other routines (though I haven't checked that this would allow astropy to remove the overrides; note that in principle we should have overrides also on our Masked class but I never got around to implementing those). It may well be that using asanyarray causes more problems, though, since np.histogram is much more used. So, that argues perhaps more for asarray here.

Anyway, regardless of which way one goes, I think it would make sense to try to actually unify the checking code, maybe by splitting out a _asarray_and_check_weights from _ravel_and_check_weights and then use the former in the latter, adding only the .ravel(), as well as in histogramdd.

mhvk · 2026-09-20T21:35:15Z

        # Sample is a sequence of 1D arrays.
        sample = np.atleast_2d(sample).T
        N, D = sample.shape
+    sample = np.asarray(sample)


The logic of astropy is that if there are no weights, the functions produce counts which should be integer arrays and thus are illogical as quantities (histogramdd appears to be inconsistent in producing float, but that is outside of astropy's control), while if there are weights, the counts get multiplied by it, so it should get the unit of the weight. The choice made is to not specifically check whether the weight itself is in fact a quantity; for float, this does not really matter, since a dimensionless quantity and float ndarray behave the same, but for integer weights arguably it is not quite right, in that one would like to get an integer array out.

ikrommyd · 2026-09-20T22:03:57Z

I don't think histogramming should preserve subclasses in general. Histogramming returns something fundamentally of a different type. Perhaps for the edges maybe it could but for the counts definitely not. Making counts also preserve subclasses would mean making np.bincount preserve them or view them in the histogramming functions. Making histogramming preserve subclasses for the edges is easy though (just an asanyarray call). But its also breaking, np.histogram(np.matrix([[1,2],[3,4]])) stops working for example.

However, personally think working with base ndarrays is the right call overall. I'm not sure preserving the subclass for the edges is the right general call. Also going towards preserving subclasses would also probably mean we need a mask-aware implementation which is a feature request on an open issue for many years.

In any case, I'm a fan of base ndarrays for histogramming and letting subclass implementers (like astropy and unyt) override so I'd probably want to go towards just refactoring out the input parsing into a helper for all the histogramming functions although I'm also okay with the 1-line change I have already.

seberg · 2026-09-21T08:21:59Z

Histogramming returns something fundamentally of a different type.

I have to think about it once more. But one can see this in all directions. If MyArr does nothing but add a few extra attributes or change how it's printed then preserving that is fine ;).

And there is a plausible middle path here, where we use _array_converter, work with base-class arrays internally and then just apply __array_wrap__ at the very end.

ikrommyd · 2026-09-21T08:27:02Z

I hope we do agree that the counts have to be base ndarray though.

mhvk · 2026-09-21T12:03:48Z

I hope we do agree that the counts have to be base ndarray though.

Only if there are no weights, otherwise they could reasonably get the subclass of the weights (as they do for Quantity), since the counts are multiplied by those.

But that is all "in principle" -- there is also "refuse the temptation to guess". I do still think that it is OK to follow your original logic and just make histogramdd use base arrays since the others do so already.

ikrommyd · 2026-09-21T12:10:57Z

I think for Quantity I'd get unitless Quantity counts with just plain python list weights though. Perhaps that makes sense. Anyways, I'm not fully thinking of edge cases here but there's potentially an argument to be made that you don't always know what the output type is if you use weights and maybe there's edge cases where using the output type of the weights is wrong but sure. I marked this as "needs decision".

mhvk · 2026-09-21T13:04:54Z

Weights can have units. E.g., if one is histogramming measurements, then least-squares weights would be 1/sigma**2, where sigma is the uncertainty which would have the unit of whatever is being measured.

ngoldbaum · 2026-09-21T15:17:52Z

Anyway, regardless of which way one goes, I think it would make sense to try to actually unify the checking code, maybe by splitting out a _asarray_and_check_weights from _ravel_and_check_weights and then use the former in the latter, adding only the .ravel(), as well as in histogramdd.

I agree with this, I'm OK with a bigger diff if it unifies the code paths than handle this conversion.

ikrommyd · 2026-09-23T19:55:53Z

The concensus from the community meeting was that we should probably go with base ndarrays. It would be nice to preserve units some times but it doesn't always work. For example if we wrap an np.matrix, we'd get 2D edges (doesn't make sense). Also it wouldn't be nice to add yet another place where __array_wrap__ is used when __array_function__ is just better. We should just do base ndarrays here and let other libraries use __array_function__ to override like astropy and unyt already do. I'll be refactoring the input parsing into a common helper to use across all 3 histogramming functions.

mhvk · 2026-09-23T20:17:19Z

@ikrommyd - yes, makes sense. For astropy, as I suspect most other projects where ndarray subclasses are used, this will add no extra burden, since we use __array_function__ already anyway to override.

ikrommyd · 2026-09-23T20:22:07Z

I don't understand why histogram2d lives in _twodim_base_impl.py though and not with the other histogramming functions in their file. Can I move it since its internal? 🤣 Unless you expect someone to be importing from that file haha

Signed-off-by: Iason Krommydas <[email protected]>

ikrommyd · 2026-09-23T21:42:37Z

I don't understand why histogram2d lives in _twodim_base_impl.py though and not with the other histogramming functions in their file. Can I move it since its internal? 🤣 Unless you expect someone to be importing from that file haha

okay I did not do that because it would be a large diff and I found an old comment (2018) from Eric where this wasn't done just because hisrogram2d is just a very very thin wrapper.

ikrommyd · 2026-09-23T21:43:15Z

    if N not in {1, 2}:
-        xedges = yedges = asarray(bins)
-        bins = [xedges, yedges]
+        bins = [bins, bins]


This should be fine I think as right below histogramdd will run asarray on whatever its given.

ikrommyd · 2026-09-23T21:46:03Z

Let me know of what you think of the structure now with the helpers. I also added tests for subclasses.

BUG: convert to plain ndarray in histogramdd

2fb428e

Signed-off-by: Iason Krommydas <[email protected]>

github-actions Bot added the 00 - Bug label Sep 19, 2026

MNT: add release note

a5dd80d

Signed-off-by: Iason Krommydas <[email protected]>

ikrommyd changed the title ~~BUG: convert to plain ndarray in histogramdd as its docstring and other histogramming functions promise~~ BUG: convert to plain ndarray in np.histogramdd Sep 19, 2026

seberg reviewed Sep 20, 2026

View reviewed changes

MNT: review comments

2921250

Signed-off-by: Iason Krommydas <[email protected]>

mhvk reviewed Sep 20, 2026

View reviewed changes

ikrommyd added 54 - Needs decision triage review Issue/PR to be discussed at the next triage meeting labels Sep 21, 2026

ngoldbaum removed the triage review Issue/PR to be discussed at the next triage meeting label Sep 23, 2026

MNT: swithc to a helper

10bc86d

Signed-off-by: Iason Krommydas <[email protected]>

ikrommyd commented Sep 23, 2026

View reviewed changes

ikrommyd removed the 54 - Needs decision label Sep 24, 2026

		@@ -0,0 +1 @@
		* `np.histogramdd` now converts and returns a plain ndarray as its docstring promises in all cases. Previously it only did that when it was handled a sequence of arrays.

Uh oh!

Conversation

ikrommyd commented Sep 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR summary

AI Disclosure

Uh oh!

ikrommyd commented Sep 19, 2026

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

seberg Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

ikrommyd Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

seberg Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

ikrommyd Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

ikrommyd commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikrommyd commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

ikrommyd commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg commented Sep 21, 2026

Uh oh!

ikrommyd commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Sep 21, 2026

Uh oh!

ikrommyd commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Sep 21, 2026

Uh oh!

ngoldbaum commented Sep 21, 2026

Uh oh!

ikrommyd commented Sep 23, 2026

Uh oh!

mhvk commented Sep 23, 2026

Uh oh!

ikrommyd commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikrommyd commented Sep 23, 2026

Uh oh!

ikrommyd Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikrommyd commented Sep 23, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ikrommyd commented Sep 19, 2026 •

edited

Loading

ikrommyd Sep 20, 2026 •

edited

Loading

ikrommyd commented Sep 20, 2026 •

edited

Loading

ikrommyd commented Sep 20, 2026 •

edited

Loading

ikrommyd commented Sep 20, 2026 •

edited

Loading

ikrommyd commented Sep 21, 2026 •

edited

Loading

ikrommyd commented Sep 21, 2026 •

edited

Loading

ikrommyd commented Sep 23, 2026 •

edited

Loading

ikrommyd Sep 23, 2026 •

edited

Loading