DEP: lib: Deprecate acceptance of float (and more) in bincount.#27076
DEP: lib: Deprecate acceptance of float (and more) in bincount.#27076seberg merged 4 commits intonumpy:mainfrom
Conversation
The first argument of bincount is expected to contain integers. As noted in numpygh-3138, it actually accepts floats, and casts them to integers with no warnings. (It also accepts other objects that can be cast to integers, so inputs such as ["1", Fraction(5, 3)] are accepted, with fractional parts silently dropped.) This change deprecates that behavior. Now a deprecation warning is generated if the input cannot be safely cast to integer. Closes numpygh-3138.
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Thanks, a few smaller comments. Could you make sure we have a test for the empty list path? (That may or may not already exist.)
| if (tmp1 == NULL) { | ||
| goto fail; | ||
| } | ||
| if (PyArray_SIZE(tmp1) > 0) { |
There was a problem hiding this comment.
One day maybe we should have a pattern for this. Indexing uses this also once for example. (We may even have lower machinery mostly, at least after deprecation.)
There was a problem hiding this comment.
I'm sure you've thought about this more than I have, but in this case, it looks like a variation of PyArray_FromAny that had one more argument, say default_dtype, that sets the dtype when the input turns out to be an array with size 0 (i.e. when the input is an empty list, or a list of empty lists, etc.) would be useful.
| } | ||
|
|
||
| /* Legacy conversion. This must be changed when the deprecation expires. */ | ||
| lst = (PyArrayObject *)PyArray_ContiguousFromAny(list, NPY_INTP, 1, 1); |
There was a problem hiding this comment.
No need for tmp2 above, just use lst there. Otherwise, you are unnecessary converting a second time here most of the time. I guess just use if (lst == NULL)` here.
There was a problem hiding this comment.
I'm not sure if this is what you are suggesting, but I can't just replace the variable tmp2 (sorry for the unimaginative names) with lst, because that wouldn't preserve the existing behavior (e.g. bincount([1.2, 2.4]) must still work).
Roughly, the added check for deprecated behavior is checking for the error that would result from changing bincount(list) to bincount(np.array(list)).
What would be better is if instead of creating tmp2 with PyArray_ContiguousFromAny, I could check that tmp1 was an integer type that I know would be safely castable to NPY_INTP, without actually making the call PyArray_ContiguousFromAny((PyObject *)tmp1, NPY_INTP, 1, 1). But after a little bit of digging (probably not enough), the correct way to do that wasn't obvious, so I decided to just make the call.
There was a problem hiding this comment.
Ah, wait, I see what you're saying. If tmp2 succeeds, just use it below. That doesn't result in any change of behavior.
There was a problem hiding this comment.
What I mean is:
if logic:
lst = new_conversion(...)
if lst == NULL:
do things...
# lst may not be defined yet, if not convert the old way:
if lst == NULL:
lst = np.asarray(list, dtype=np.intp)
Done. There were tests for an empty array, but not for an empty list. |
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Looks fine, thanks. When the deprecation is finalized, it would be nice to clean this up a bit more (the try/except feels a bit round-about, but good enough for a deprecation).
seberg
changed the title
|
Ah, it's pretty darn niche, but maybe we should add a bullet deprecation release note snippet? Sorry for forgetting, do you want to do that? |
|
Thanks @seberg. I'll follow up with a release note. |
[skip actions] [skip azp] [skip cirrus]
DOC: Add release note about deprecation introduced in gh-27076.
…y#27076) The first argument of bincount is expected to contain integers. As noted in numpygh-3138, it actually accepts floats, and casts them to integers with no warnings. (It also accepts other objects that can be cast to integers, so inputs such as ["1", Fraction(5, 3)] are accepted, with fractional parts silently dropped.) This change deprecates that behavior. Now a deprecation warning is generated if the input cannot be safely cast to integer. Closes numpygh-3138. Co-authored-by: Sebastian Berg <[email protected]>
[skip actions] [skip azp] [skip cirrus]
2 participants
The first argument of bincount is expected to contain integers. As noted in gh-3138, it actually accepts floats, and casts them to integers with no warnings. (It also accepts other objects that can be cast to integers, so inputs such as ["1", Fraction(5, 3)] are accepted, with fractional parts silently dropped.)
This change deprecates that behavior. Now a deprecation warning is generated if the input cannot be safely cast to integer.
For example,
Closes gh-3138.