API: Use ResultType in PyArray_ConvertToCommonType#14933
Conversation
60cabfc to
154a6c3
Compare
|
This is a slight API change... SciPy does not use the function at all, we use it in a single place (choose), where I think this can be considered a bug fix. So I tend to think we can just pull this off? Removing this may allow to remove some "scalar casting" logic, which I am not sure is much code, but is just confusing in any case (since it is a complete second system next to the typical "value based scalar casting/promotion" of result type; even if in principle it is exposed to custom DTypes and as slots, on the dtypes, I would be extremely surprised if anyone uses it). We do seem to have a few other remnants of this in EDIT: OK, the thing in |
154a6c3 to
a2bce42
Compare
|
OK, I think I am leaning towards just doing this, but to be honest. Keeping things as they are is probably just a mild annoyance. It is simple logic for builtin dtypes, and for user dtypes we will just phase out the logic in any case. |
|
Needs a release note. Might have backwards compatibility implications. |
a2bce42 to
b56691d
Compare
doc/source/reference/c-api/array.rst
Outdated
There was a problem hiding this comment.
"in the same way as PyArray_ResultType."
doc/source/reference/c-api/array.rst
Outdated
There was a problem hiding this comment.
A mix of scalars and zero-dimensional arrays now produces a type capable of holding the scalar value. Previously priority was given to the dtype of the arrays.
There was a problem hiding this comment.
maybe "correct arrays" -> PyArrayObjects
|
+1 for simplifying the code and being more consistent. Does this affect pandas or astropy? |
This slightly modifies the behaviour of `np.choose` (practically a bug fix) and the public function itself. The function is not used within e.g. SciPy, so the small performance hit of this implementation is probably insignificant. The change should help clean up dtypes a bit, since the whole "scalar cast" logic is brittle and should be deprecated/removed, and this is probably one of the few places actually using it. The choose change means that: ``` np.choose([0], (1000, np.array([1], dtype=np.uint8))) ``` will actually return a value of 1000 (the dtype not being uint8).
b56691d to
c5da77d
Compare
|
Well, they would be affected by the side effect of changing |
|
From what I can tell, pandas tests are unaffected by this change. |
|
@mhvk do you have an idea if this will affect astropy? |
|
Thanks @seberg. |
This slightly modifies the behaviour of
np.choose(practically abug fix) and the public function itself. The function is not used within
e.g. SciPy, so the small performance hit of this implementation
is probably insignificant.
The change should help clean up dtypes a bit, since the whole "scalar
cast" logic is brittle and should be deprecated/removed, and this is
probably one of the few places actually using it.
The choose change means that:
will actually return a value of 1000 (the dtype not being uint8).
EDIT: To be clear, the "performance hit" is since there can be multiple conversions to array, or an unecessary copy/cast in principle.