BUG: `take` casting logic with an `out=` argument #4246

jaimefrio · 2014-01-30T21:46:39Z

When it receives an out argument, np.take converts the out array to
the dtype of the array being taken from. This results in wrong TypeErrors
being raised when out has a larger dtype than the array (and not being
raised when it has a smaller dtype, even though they should):

>>> a = np.arange(3, dtype=np.int16) + 128
>>> b = np.empty((3,), dtype=np.int8)
>>> a.take([0, 1, 2], out=b) # wrong, should raise an error
array([-128, -127, -126], dtype=int8)
>>> b = np.empty((3,), dtype=np.int32)
>>> a.take([0, 1, 2], out=b) # perfectly OK, but raises an error
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Cannot cast array data from dtype('int32') to dtype('int16') according to the rule 'safe'

This PR makes an explicit casting check, then either sets NPY_ARRAY_FORCECAST
or explicitly raises the error.

The tests added check the logic for conversion between all pairs of ints and
floats only.

charris · 2014-04-04T00:01:17Z

numpy/core/src/multiarray/item_selection.c

+         */
+        if (PyArray_CanCastTypeTo(dtype, out_dt, NPY_SAFE_CASTING)) {
+            flags |= NPY_ARRAY_FORCECAST;
+        } else {


PEP7

} else {

charris · 2014-04-04T01:18:23Z

The logic of this function is strange, the out parameter isn't even used unless no copy is made in producing obj, and I don't see a warning when it isn't used. I guess it is like ravel in that respect.

When it receives an `out` argument, `np.take` converts the `out` array to the dtype of the array being taken from. This results in wrong TypeErrors being raised when `out` has a larger dtype than the array (and not being raised when it has a smaller dtype, even though they should). This PR sets NPY_ARRAY_FORCECAST and makes an explicit casting check, raising an error if it fails. The tests added check the logic for conversion between all pairs of ints and floats only.

jaimefrio · 2014-04-06T05:22:50Z

I have simplified things as much as possible following your suggestions. The comment in the code is still kind of long and unclear. Perhaps it is better to remove it completely and let the code speak for itself?

charris · 2014-04-07T02:56:24Z

numpy/core/src/multiarray/item_selection.c

+         * is needed, as the constructor of obj would check the opposite
+         * casting, were it not disabled by the NPY_ARRAY_FORCECAST flag.
+         */
+        if (PyArray_CanCastTypeTo(dtype, PyArray_DESCR(out),


I'm thinking this check is incorrect. It was previously allowed to cast, say, float64 to int8, and ufuncs also allow such casting. The forcecast going the other way should be the only change needed as it provides a type compatible work array, which is then copied back on success or discarded on failure. The test could check that out remains unchanged when the mode is raise and an indexing error occurs.

charris · 2014-04-07T03:07:01Z

This looks much cleaner, but I don't think it is quite correct yet.

jaimefrio · 2014-04-07T03:53:18Z

I just did a little bit of archeology, and we had this same conversation a little over a year ago:

http://mail.scipy.org/pipermail/numpy-discussion/2013-February/065441.html

It is indeed true that ufuncs will allow any type of casting to out, wasn't fully aware of that, so reproducing that by simply adding the NPY_ARRAY_FORCECAST, plus the testing to avoid regressions is probably the right thing to do.

jaimefrio · 2014-04-07T04:18:00Z

When simply forcing the cast it raises a different error:

======================================================================
FAIL: Ticket #789, changeset 5217.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\WinPython-32bit-np-dev\python-2.7.5\lib\site-packages\numpy\core\test
s\test_regression.py", line 1027, in test_compress_small_type
    raise AssertionError("compress with an out which cannot be " \
AssertionError: compress with an out which cannot be safely casted should not return successfully

Apparently it is expected that compress does not behave like ufuncs, and it was relying on take to enforce that behavior. The test for compress works by pure chance: not because int cannot be cast to single, but because single cannot be cast to int.

Since compress has been broken all this time the same way that take was, and no one else has complained, it is tempting to remove the test...

seberg · 2014-04-07T11:09:22Z

Only half true I believe, isn't that behaviour in ufuncs is deprecated I believe.

charris · 2014-04-09T00:58:40Z

@seberg Hmm, need to check that. I'm concerned about changing functions in the API even if it looks like this function isn't much used ;) Currently, the function only works if the output precision is <= input precision, with this PR it is >= instead.

seberg · 2014-04-09T04:58:22Z

Wasn't arguing about just using forced casting here. I think it should be good. Could or could not add deprecations on top of that then, just like the ufuncs have right now. Or similar to them anyway.

jaimefrio · 2014-04-09T06:41:06Z

The forced cast without type checking raises that error in compress. Apparently someone was expecting compress to not simply force a cast to the out type, but do what this PR originally did . If we go that way, then the test in compress has to be removed. Which is a good idea anyway, because it is testing the wrong thing.

I haven't been able to find "Ticket #789, changeset 5217" to see if there is a deeper reason for that test, any clue?

seberg · 2014-04-09T09:26:03Z

Well, that test tests a segfault bug (see gh-1387), so there is no need to see it as someone wanted to fix behaviour there. We are not awfully consistent about what we allow with the out argument anyway. Some allow forced casting, some want exact matching, and for ufuncs there is a plan to make it safe...

charris · 2014-04-11T00:42:06Z

Looking at scipy, the out parameter is never used in calls to take. I'm thinking it is probably safe (and a good idea) to check the out type. Ideally we would add a casting argument, but that can't be done at the C level. Hmm, How about we get rid of the check here, and add it to the python take function along with the casting option?

charris · 2014-04-11T00:49:09Z

Actually, array_take in methods.c looks like the best place to add an option.

charris · 2014-04-11T00:57:33Z

There is PyArray_CastingConverter available to check the conversion and array_astype provides a template for the usage.

charris · 2015-02-27T02:51:06Z

Maybe time to take another look at this.

homu · 2015-07-25T20:00:48Z

☔ The latest upstream changes (presumably #6047) made this pull request unmergeable. Please resolve the merge conflicts.

seberg · 2019-04-26T19:11:06Z

@jaimefrio this seems to still requires some work. Unless you comment/wish to continue, I would probably:

close the PR
create a new issues to note that there is a start for fixing this in this PR. (Or modify the existing one)

in a few weeks. Of course the PR can be reactivated at any time. (tag:PR-possibly-close)

In general this seems like a fairly straight forward task, although likely requiring some form of Deprecation.

anirudh2290 · 2020-05-20T20:40:13Z

This has been stale from the point when @seberg last responded. Closing for now. Feel free to reopen @jaimefrio if you still wish to pursue this task.

eric-wieser · 2020-05-20T20:42:12Z

I think we should create an issue to continue tracking this behavior

anirudh2290 · 2020-05-20T20:43:10Z

sure @eric-wieser, will do.

charris reviewed Apr 4, 2014
View reviewed changes

charris reviewed Apr 7, 2014
View reviewed changes

rgommers added 03 - Maintenance component: numpy._core labels Mar 8, 2015

anirudh2290 closed this May 20, 2020

anirudh2290 mentioned this pull request May 20, 2020

BUG: take casting logic with an out= argument #16319

Open

lorentzenchr mentioned this pull request Jan 17, 2024

BUG: np.take cast to out argument #25588

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: `take` casting logic with an `out=` argument #4246

BUG: `take` casting logic with an `out=` argument #4246

jaimefrio commented Jan 30, 2014

charris Apr 4, 2014

charris commented Apr 4, 2014

jaimefrio commented Apr 6, 2014

charris Apr 7, 2014

charris commented Apr 7, 2014

jaimefrio commented Apr 7, 2014

jaimefrio commented Apr 7, 2014

seberg commented Apr 7, 2014

charris commented Apr 9, 2014

seberg commented Apr 9, 2014

jaimefrio commented Apr 9, 2014

seberg commented Apr 9, 2014

charris commented Apr 11, 2014

charris commented Apr 11, 2014

charris commented Apr 11, 2014

charris commented Feb 27, 2015

homu commented Jul 25, 2015

seberg commented Apr 26, 2019 •

edited

Loading

anirudh2290 commented May 20, 2020

eric-wieser commented May 20, 2020

anirudh2290 commented May 20, 2020

BUG: take casting logic with an out= argument #4246

BUG: take casting logic with an out= argument #4246

Conversation

jaimefrio commented Jan 30, 2014

charris Apr 4, 2014

Choose a reason for hiding this comment

charris commented Apr 4, 2014

jaimefrio commented Apr 6, 2014

charris Apr 7, 2014

Choose a reason for hiding this comment

charris commented Apr 7, 2014

jaimefrio commented Apr 7, 2014

jaimefrio commented Apr 7, 2014

seberg commented Apr 7, 2014

charris commented Apr 9, 2014

seberg commented Apr 9, 2014

jaimefrio commented Apr 9, 2014

seberg commented Apr 9, 2014

charris commented Apr 11, 2014

charris commented Apr 11, 2014

charris commented Apr 11, 2014

charris commented Feb 27, 2015

homu commented Jul 25, 2015

seberg commented Apr 26, 2019 • edited Loading

anirudh2290 commented May 20, 2020

eric-wieser commented May 20, 2020

anirudh2290 commented May 20, 2020

BUG: `take` casting logic with an `out=` argument #4246

BUG: `take` casting logic with an `out=` argument #4246

seberg commented Apr 26, 2019 •

edited

Loading