ENH: Allow bincount(..., minlength=0). #8348
Conversation
|
What is "this". The description is seriously lacking in detail. |
|
And the commit message also. |
|
Currently |
| @@ -116,10 +116,10 @@ arr_bincount(PyObject *NPY_UNUSED(self), PyObject *args, PyObject *kwds) | |||
| } | |||
| else { | |||
| minlength = PyArray_PyIntAsIntp(mlength); | |||
There was a problem hiding this comment.
I suppose we can't do it because backwards compatibility... But the temptation is strong to change the PyArg_ParseTupleAndKeywords format string to "O|On" and get rid of the mlength variable and the None handling logic altogether...
There was a problem hiding this comment.
It is very strong. Note that I did change the docstring to pretend this is the case...
There was a problem hiding this comment.
Yes, I did notice, much better default if you ask me.
There was a problem hiding this comment.
Would be great to add a deprecation warning for passing None here, so that down the line we have an option to remove it
|
You have a bunch of failing tests because you did not update them to match the new string of the exception (positive --> non-negative). Other than that, and my inline comment on how I wish we could stop supporting |
|
fixed |
anntzer
force-pushed
the
bincount-zero-minlength
branch
from
f9213e1
to
1bf8d67
Compare
|
@jaimefrio we can probably merge this, or do you want to add a deprecation? |
| @@ -2374,6 +2374,8 @@ def test_with_minlength_smaller_than_maxvalue(self): | |||
| x = np.array([0, 1, 1, 2, 2, 3, 3]) | |||
| y = np.bincount(x, minlength=2) | |||
| assert_array_equal(y, np.array([1, 2, 2, 2])) | |||
| y = np.bincount(x, minlength=0) | |||
| assert_array_equal(y, np.array([1, 2, 2, 2])) | |||
There was a problem hiding this comment.
Needs a test that np.bincount([], minlength=0) actually returns an empty list
eric-wieser
added
01 - Enhancement
component: numpy._core
56 - Needs Release Note.
|
Commit message should start |
eric-wieser
changed the title
anntzer
force-pushed
the
bincount-zero-minlength
branch
from
1bf8d67
to
ec6d429
Compare
|
done |
|
Looks good - two more things:
|
|
Feel free to pick the PR up if you want to add a deprecation warning, as I would consider it a separate issue. |
DOC: Update bincount docs to reflect gh-8348 (backport)
DOC: Update bincount docs to reflect gh-8348
I don't see a reason why this shouldn't be allowed.
This should have the semantics that
bincount(..., minlength=None)currently has