Skip to content

ENH: add an axis argument to np.bincount #4330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

jaimefrio
Copy link
Member

This adds an axis argument to np.bincount, which can now take multidimensional
arrays and do the counting over an arbitrary number of axes. axis defaults
to None, which does the counting over all the axes, i.e. over the flattened
array.

The shape of the output is computed by removing from list (the input array)
all dimensions in axis, and appending a dimension of length
max(max(list) + 1, minlength) to the end. out[..., n] will hold the number
of occurrences of n at the given position over the selected axes.

If a weights argument is provided, its shape is broadcasted with list
before removing the axes. In this case, axis refers to the axes of list
before broadcasting, and out[..., n] will hold the sum of weights over
the selected axes, at all positions where list takes the value n.

The general case is handled with nested iterators, but shortcuts without
having to set up an iterator are provided for 1D cases, with no performance
loss against the previous version.

As a plus, this PR also solves #823, by providing specialized functions for
all integer types to find the max. There are also specialized functions for
all integer types for counting and doing weighted counting.

This adds an axis argument to np.bincount, which can now take multidimensional
arrays and do the counting over an arbitrary number of axes. `axis` defaults
to `None`, which does the counting over all the axes, i.e. over the flattened
array.

The shape of the output is computed by removing from `list` (the input array)
all dimensions in `axis`, and appending a dimension of length
`max(max(list) + 1, minlength)` to the end. `out[..., n]` will hold the number
of occurrences of `n` at the given position over the selected axes.

If a `weights` argument is provided, its shape is broadcasted with `list`
before removing the axes. In this case, `axis` refers to the axes of `list`
*before* broadcasting, and `out[..., n]` will hold the sum of `weights` over
the selected axes, at all positions where `list` takes the value `n`.

The general case is handled with nested iterators, but shortcuts without
having to set up an iterator are provided for 1D cases, with no performance
loss against the previous version.

As a plus, this PR also solves numpy#823, by providing specialized functions for
all integer types to find the max. There are also specialized functions for
all integer types for counting and doing weighted counting.
@@ -4987,48 +4987,58 @@ def luf(lamdaexpr, *args, **kwargs):

add_newdoc('numpy.lib._compiled_base', 'bincount',
"""
bincount(x, weights=None, minlength=None)
bincount(list, weights=None, minlength=None, axis=None)
Copy link
Member

Choose a reason for hiding this comment

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

I think list is an unfortunate choice for a variable name, x was fine, but maybe arr or a instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was basically trying to make explicit the following current behavior:

>>> np.bincount(list=[3,4,5])
array([0, 0, 0, 1, 1, 1], dtype=int64)

Internally, the first argument is a kwarg with label list. I agree that it is an unfortunate choice, and will change it both internally and in the docstring to arr. Since it is an undocumented feature, I understand this change can be made with out worries, right?

@charris
Copy link
Member

charris commented Feb 22, 2014

I bailed early here ;) looking at the code size here relative to the original function, I prefer the original. The use of type specific code complicates things enormously. A simple, if not so fast, approach, would be to do this in the python call, just calling bincount in a loop and using max to set up the output array shape.

I guess I'm saying this is a premature optimization ;)

This does suggest we could use a standard function dispatcher.

@charris
Copy link
Member

charris commented Feb 22, 2014

To avoid arr_bincount doing extra work, we could add a argument to set the length and discard any bins greater than that. Maybe clip, meaning minlength is also a maxlength.

@jaimefrio
Copy link
Member Author

Yes, I probably got a little carried away optimizing performance once I got it running. The type specific code, is there to keep the performance for small arrays vs the old function. There was also the thing of handling uint64 inputs, but there have to be other options if that is all you want done. If you think this is bad, you should have seen it with specialized loops for the weights types, to allow complex numbers there also! ;-)

It is very easy to not do any of this and simply have the iterator buffer and cast everything to npy_intp/npy_double and have a single inlined loop. There is a performance hit for small arrays in setting up the iterator, which then run about 2x slower, but making contiguous copies of 1D arrays if they are not behaved/of the right type will probably take care of most of that.

Let me try to rewrite with a leaner approach: I promise you want have to read over so much code!

@charris
Copy link
Member

charris commented Feb 22, 2014

I think a clip keyword might be useful in any case.

@jaimefrio
Copy link
Member Author

I have just pushed a slimmed down version of ndbincount. As is, it has all the bells and whistles of broadcasting using the axis parameter, but is comparably as fast as the original version for 1D arrays. It could be simplified a little by always using the iterator, but then it would be 2x slower than the original code for small arrays. It's still several hundred more lines of code...

@josef-pkt
Copy link

Just as cheerleader for this since another issue reminded me of it:

statsmodels would have quite a lot of use for a vectorized version of bincount. I always missed that.
I never hit the "send" button for my reply almost a year ago. The main questions were about broadcasting which I don't know whether they are still relevant.
In any case, I and we can adjust to whatever the pattern numpy has now for partial reduction, and we will use any vectorized version if it's more efficient than the plain loops.

@jaimefrio
Copy link
Member Author

I am closing this for now. There are several functions out there that could use an axis argument, and bincount is one of them. But rather than bloating them all with code, the proper approach seems to me to build some intermediate layer over nditer that abstracts the complexity away. It would probably make for a nice GSoC project...

@jaimefrio jaimefrio closed this Mar 9, 2015
@rgommers
Copy link
Member

rgommers commented Mar 9, 2015

@jaimefrio you could still consider the suggestion of @charris to do this in a for-loop in Python. That would keep the code simple, and non-optimal speed for something that doesn't work at all now is OK:) Reason to do that: the intermediate layer might take a long time to materialize, and this does look useful.....

@mhvk
Copy link
Contributor

mhvk commented May 22, 2016

The mailing list conversation reminded me: the multidimensional weight, single-dimension index case of this was something I was very much looking forward to. I can see why this was closed, but it would still be really great to have...

@eric-wieser
Copy link
Member

Relevant to #5197

@mhvk
Copy link
Contributor

mhvk commented Mar 25, 2017

Would still be great to have this -- @eric-wieser - np.bincount is very fast; we'd have to be very careful about performance before thinking of moving this to a gufunc (I analyze GBs of baseband data with it, so I care...).

@eric-wieser
Copy link
Member

I would hope that the cost of a gufunc would only be a small constant, whereas it sounds like you're using it on large inputs anyway?

@mhvk
Copy link
Contributor

mhvk commented Mar 25, 2017

True, I just worry about the iterator not being optimized as much, but with a good inner loop I guess it should be fine.

@eric-wieser
Copy link
Member

I just worry about the iterator not being optimized as much,

As much as what? Obviously iterating over something takes more time than assuming it has length 1...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants