TST: np.take out dtype #25600
TST: np.take out dtype #25600
Conversation
|
@charris I first wanted the CI to finish to prove that the test triggers a failure. |
| @@ -303,7 +303,7 @@ PyArray_TakeFrom(PyArrayObject *self0, PyObject *indices0, int axis, | |||
| */ | |||
| flags |= NPY_ARRAY_ENSURECOPY; | |||
| } | |||
| dtype = PyArray_DESCR(self); | |||
| dtype = PyArray_DESCR(out); | |||
There was a problem hiding this comment.
Tests are failing, but that isn't surprising. The dtype must be self, because this ensures that the core operation happens with a single identitcal dtype (the input arrays, at the cost of a copy if needed).
In either case, this sis just not right. What is probably required is to add force-casting to the flags NPY_FORCECAST(?). But additionally add an explicit cast safety check.
TBH, there should be many other functions that implement out= which get it right.
There was a problem hiding this comment.
If you can point me to a function from where to steal that would help me a lot.
There was a problem hiding this comment.
Not sure how consistent we are. There are two possibilities:
- You allow same-kind casts (probably), but make sure the direction of the cast check is correct.
- You only allow equivalent/no casts.
I don't care much, probably 1. is just as well, we tend to copy anyway often enough, and frankly, you could probably integrate the casting intot he core loop in the future in principle.
I think this should be OK as a (admittedly a bit rough) example: https://github.com/numpy/numpy/blob/main/numpy/_core/src/multiarray/multiarraymodule.c#L592
It doesn't use writeback if copy, but that logic should be fine (except forcing the cast).
There was a problem hiding this comment.
After seeing that take is used internally in a few more "ufunc like" operations, and those use same-kind casting (actually we can also use NPY_DEFAULT_ASSIGN_CASTING or so in C:
I would lean with going with that same-kind casting here.
This appears to not just fix take, but a chain of other operations. Might be worthwhile to try to finish it.
The example I gave actually gives a DeprecationWarning which would make sense (unlike ufuncs, etc. we are not planning on casting= to allow a very simple resolution for all the places this kicks in.)
But for 2.0, I can also see just doing away with this: It is highly untypical for NumPy in other places.
There was a problem hiding this comment.
What is your exact proposal? What should be done?
The 8000 re was a problem hiding this comment.
I would go with testing for same-kind casting of the output, and otherwise give a DeprecationWarning, since it isn't a big hassle to do that.
There was a problem hiding this comment.
Without clearer instructions, I'm going to have to close this PR.
Fixes gh-25588, gh-16319, gh-22766, and gh-21676
I'm a numpy newbie. While this PR adds a test case and makes the added test pass, I don't fully see all the consequences of my changes, in particular how
PyArray_FromArrayworks.