BUG: ensure that casting to/from structured is properly checked. #13648

mhvk · 2019-05-28T01:44:43Z

As noted in #11114, currently the following clearly wrong things happen:

np.can_cast(1., 'i2,i2', casting='safe')
# True
# while
np.can_cast(1., 'i2', casting='safe')
# False
np.array(1000.).astype('i1,i1', casting='safe')
# array((-24, -24), dtype=[('f0', 'i1'), ('f1', 'i1')])
# while
np.array(1000.).astype('i1', casting='safe')
# TypeError: Cannot cast array from dtype('float64') to dtype('int8') according to the rule 'safe'

The present PR fixes it, and has some elementary tests. In particular, it allows casting from simple to structured (EDIT) in unsafe mode, and from structured to simple only in unsafe mode and when the structured dtype has only a single field.

fixes #11114

p.s. Replaces #13646, which confused the general void type with structured.

eric-wieser · 2019-05-28T04:14:43Z

numpy/core/src/multiarray/convert_datatype.c

+            }
+            else {
+                return 0;
+            }


I could perhaps see an argument that in this direction, this should be any(PyArray_CanCastTypeTo(from, field, casting) for field in fields), allowing 2 to cast into (2, 2, 2,), which I think we have precedent for

I was perhaps more driven by symmetry here than anything else. But I think you are right: casting to multiple fields is in principle not ambiguous. But it should be all(cancast(...)), then, not any(...), correct?

To take a step back here, do we want safe casting? It feels right, but it also feels right that safe casting in most cases aligns with "common_type" and I am not sure I feel that I feel things like np.concatenate should feel they can just "broadcast" to all fields without some manual casting. (I did not make up my mind yet, probably its OK).

To me it's a question of if we already set a precedent for this weird behavior. If we didn't, then the original more conservative patch was probably the right one.

I'm moving this out of the in-line comments, so it doesn't get lost when I change things.

mhvk · 2019-05-29T01:06:51Z

@eric-wieser - I changed it to allow casting to multiple fields. Also realized I had to put it further up, since the changes hold also for unsafe casting. Since I think this is getting close, I also updated the docstrings. I think can_cast is now consistent with actually executing a.astype(...).

seberg

I suppose a few tests assume that unsafe casting works in strange ways... I am not sure we want to go there and change that just now...

numpy/core/_add_newdocs.py

numpy/core/multiarray.py

numpy/core/src/multiarray/convert_datatype.c

seberg

I suppose a few tests assume that unsafe casting works in strange ways... I am not sure we want to go there and change that just now...

numpy/core/_add_newdocs.py

mhvk · 2019-05-29T14:01:36Z

@seberg, @eric-wieser, thanks for all the comments!

On the casting, I'm a bit torn. I agree with @eric-wieser that we should try to make can_cast consistent with actual casting behaviour in, e.g., astype, to the extent possible. The current behaviour of astype and __setitem__ (see below) can be summarized as follows:

Structured to simple: only allowed for a single field and when casting="unsafe"; can_cast is inconsistent with this in always returning True for "unsafe", independent of the number of fields. This is an easy fix.
Simple to structured: always done for any casting but "equiv" and "no"; this is the bug I hoped to fix.

My own sense is that symmetry suggests that we do not allow simple to structured at all for any casting but "unsafe". But for "unsafe", it does seem fine to just allow the cast to all fields, especially as this is what happens also in __setitem__.

This seems to make sufficient sense that I'll go ahead and change it accordingly...

Current behaviour

Going from structured to simple:

import numpy as np
# Multiple fields to simple
i8 = np.array([1000])
i1i1 = np.array([(1,2)], dtype='i1,i1')
np.can_cast('i1,i1', 'i8', casting='unsafe')
# True # and False for any other casting mode
i1i1.astype('i8', casting='unsafe')
# ValueError: Can't cast from structure to non-structure, except if the structure only has a single field.
i8[...] = i1i1
# ValueError: Can't cast from structure to non-structure, except if the structure only has a single field.

# Single field to simple
i1 = np.array([(1,)], dtype=[('i', 'i1')])
np.can_cast([('i', 'i1')], 'i8', casting='unsafe')
# True  # and False for any other casting mode
i1.astype('i8', casting='unsafe')
# array([1])  # note that casting='unsafe' is default
i8[...] = i1
# Passes -> i8=1
np.array([(1,)], dtype=[('i', 'i1')]).astype('i8', casting='safe')
# TypeError: Cannot cast array from dtype([('i', 'i1')]) to dtype('int64') according to the rule 'safe'

So, for this case, astype behaves sensibly in allowing a cast from structured to simple only when there is a single field, and even then only when casting="unsafe" - the current PR would extend this to allow any case where the single field can be cast to the simple type, but I think I should revert that and instead just make can_cast consistent with astype, i.e., change its behaviour for "unsafe" to not allow the case of multiple fields.

Now turning to casting from simple to structured

import numpy as np
i8 = np.array([1000])
# Simple to multiple, non-safe fields
np.can_cast('i8', 'i1,i1', casting='safe')
# True  # only 'no' and 'equiv' give False
i8.astype('i1,i1', casting='safe')
# array([(-24, -24)], dtype=[('f0', 'i1'), ('f1', 'i1')])
i1i1[...] = i8
# Passes -> same array.
# only 'no' and 'equiv' error
# Behaviour identical for safe fields and for a single field!

So, the current behaviour is that casting to a structured type is never allowed for "equiv" and always allowed for "safe" and beyond.

mhvk · 2019-05-29T14:12:00Z

p.s. I think there actually is another inconsistency for structured to structured casting, in that can_cast looks at field names while astype and __setitem__ go by field order. Probably a leftover of #6053, so cc @ahaldane (and let's leave it out of this PR...)

EDIT: see gh-13667

mhvk · 2019-05-29T15:19:02Z

Rewritten as noted above. Quite a bit simpler PR, which suggests it may be better..

seberg

This may get more complicating, fear... I am willing to move this forward though as an improvement though if you feel that is better. There is no regression here in 1.17, right?

seberg · 2019-05-29T21:12:56Z

numpy/core/src/multiarray/convert_datatype.c

+    /*
+     * Fast path for basic types.
+     */
+    if ((NPY_LIKELY(from->type_num < NPY_OBJECT) &&


I suppose <= would work as well, not that it matters.

This was there before, but you're right, no sense in not including object.

seberg · 2019-05-29T21:14:34Z

numpy/core/src/multiarray/convert_datatype.c

+         */
+        if (!PyDataType_HASFIELDS(to) && to->type_num != NPY_OBJECT) {
+            return (casting == NPY_UNSAFE_CASTING &&
+                    PyDict_Size(from->fields) == 1);


I wonder if Eric won't come around a corner here and spook you with a dtype with nested fields :), it seems probable that there needs to be a recursion somewhere to get it right.

You're right, this should be recursive.

seberg · 2019-05-29T21:17:22Z

numpy/core/src/multiarray/convert_datatype.c

+            return 1;
+        }
+    }
+    else if (PyDataType_HASFIELDS(to)) {


What about object and subarrays? E.g np.dtype("(1,2)u8")

Sorry, of course rather something like> np.dtype(["f1", "f4", (5,)]) or np.dtype([("f1", "f,f")])

I think for casting to structured array, sub-arrays do not matter - certainly- astype happily does it. This particular stanza is really mostly there to ensure we return False when casting is not "unsafe".

For subarrays in from, astype seems quite happy to just take the first element. So, for now just mimicking that.

What happens for:

from_dt = np.dtype([('a', ('i4,i4', 2))]) to_dt = np.dtype([('a', 'i4')])

For instance

arr = np.array(([(1, 2), (3, 4)],), dtype=from_dt) arr.astype(to_dt)

What's currently implemented is not "just take the first element", but "take the first element and ignore all casting rules when casting it"

Your particular example with astype raises an error in both in current master and in my PR, because from_dt['a'] itself has two fields, which cannot be cast to a single one.

Sadly, though, can_cast happily claims this works even in my PR as from['a'] has no fields (but does have a subdtype) - though only for casting='unsafe'.

OK, that particular case is solved. But really astype is weird - it checks can_cast internally but can still throw an error later. This should not be allowed...

Well, astype casting could fail based on the values. Of course you are right and it should not fail based on information already inside the dtype.

I have to catch up a bit here, sorry, but I do not really want crunch my mind too much about it right now, we are talking about corner cases and if we have new infrastructure things might get easier to align.

@seberg - agreed that we should take this bit-by-bit. My main goal really is to ensure that structured with multiple fields to unstructured always returns False, as this cannot possibly work (and has caused me real head-aches).

mhvk · 2019-05-29T23:59:36Z

OK, pushed another update. Am still trying to keep the impact minimal. Another worry I have is whether user types can have fields - though at least this should be better than it was before...

numpy/core/_add_newdocs.py

mhvk · 2019-05-31T17:55:34Z

Damn, astype is really quite inconsistent too in what it does for subarrays. For now, focussing on getting the structured from right...

charris · 2019-06-03T13:23:08Z

Also failing doctest at numpy.doc.structured_arrays.

mhvk · 2019-06-12T01:23:08Z

@seberg, @eric-wieser - what do you think we should do with this PR? Overall, I feel it at least fixes one obvious glaring inconsistency. On the other, it leaves many others. But those generally need more thought at developing a better set of tests. Personally, I'd prefer to approach this incrementally (mostly as I don't think I've got the time to do the larger overhaul...).

seberg · 2019-06-12T02:26:40Z

@mhvk sorry, meant to look through this for a bit now, but then did not when I saw we had put 1.18, and thinking I can't merge it anyway ;).
I hope more restructuring of casting can/will happen, but I am ok with doing some small improvements before the end. Plus anything that works and is tested now, is something we can't break when we try to do larger changes.

mhvk · 2019-06-12T13:47:06Z

Ideally this would still go in 1.17...

charris · 2019-06-12T14:51:32Z

The 1.18 just means it isn't a blocker for 1.17, it can certainly go in 1.17.

seberg

OK, sorry for that. Can't find anything to squirm about right now. But I do think we should add release notes, since casting is changed in subtle ways. As most actual casting uses "unsafe" mode, I doubt that those changes affect a lot of code though (and are not already buggy).

numpy/core/src/multiarray/convert_datatype.c

numpy/core/multiarray.py

numpy/core/_add_newdocs.py

numpy/core/tests/test_numeric.py

Allow unsafe casting from a simple data type to a structured one with multiple fields, but only from structured data type with a single field (checked recursively) to a simple data type. For now, continue to allow any structured to structured with casting="unsafe", as the current can_cast implementation does not match that of "astype".

mhvk · 2019-06-12T18:49:50Z

OK, made the changes, and added a release note.

seberg · 2019-06-12T19:11:28Z

doc/release/1.17.0-notes.rst

+Previously, ``can_cast`` returned `True` for almost all inputs for
+``casting='unsafe'``, even for cases where casting was not possible, such as
+from a structured dtype to a regular one.  This has been fixed, making it
+more consistent with actual casting using, e.g., the ``.astype`` method.


There is also the change that np.zeros(3, "i4").astype("i4,i4", casting="safe") (or related can_cast) should fail after this? But maybe it is very fringe, since astype defaults to "unsafe" in any case, and so does assignment I think.

Anyway, I am good with merging this.

Thanks, let me just merge this...

seberg · 2019-06-12T20:17:22Z

Cool, thanks Marten.

mhvk added 00 - Bug component: numpy.dtype labels May 28, 2019

mhvk mentioned this pull request May 28, 2019

WIP,BUG: ensure that casting to void is properly checked. #13646

Closed

mhvk requested a review from seberg May 28, 2019 01:46

eric-wieser reviewed May 28, 2019

View reviewed changes

mhvk force-pushed the disallow-can-cast-of-everything-to-structured branch 2 times, most recently from e41981e to 7ce1813 Compare May 29, 2019 01:05

seberg reviewed May 29, 2019

View reviewed changes

eric-wieser reviewed May 29, 2019

View reviewed changes

numpy/core/_add_newdocs.py Outdated Show resolved Hide resolved

mhvk force-pushed the disallow-can-cast-of-everything-to-structured branch from 7ce1813 to 7f15d42 Compare May 29, 2019 15:18

mhvk force-pushed the disallow-can-cast-of-everything-to-structured branch from 7f15d42 to cd90983 Compare May 29, 2019 15:22

seberg self-requested a review May 29, 2019 15:42

seberg reviewed May 29, 2019

View reviewed changes

mhvk force-pushed the disallow-can-cast-of-everything-to-structured branch from 7a2d01b to 2675ef9 Compare May 29, 2019 23:58

eric-wieser self-requested a review May 30, 2019 04:57

eric-wieser reviewed May 31, 2019

View reviewed changes

numpy/core/_add_newdocs.py Outdated Show resolved Hide resolved

mhvk force-pushed the disallow-can-cast-of-everything-to-structured branch from 2675ef9 to 09639c4 Compare May 31, 2019 17:53

mhvk force-pushed the disallow-can-cast-of-everything-to-structured branch from 09639c4 to e4efd1d Compare May 31, 2019 20:43

seberg self-requested a review June 1, 2019 00:11

charris added the 09 - Backport-Candidate PRs tagged should be backported label Jun 3, 2019

mhvk force-pushed the disallow-can-cast-of-everything-to-structured branch from 840d924 to a6f1713 Compare June 4, 2019 20:02

seberg approved these changes Jun 12, 2019

View reviewed changes

numpy/core/src/multiarray/convert_datatype.c Show resolved Hide resolved

numpy/core/multiarray.py Outdated Show resolved Hide resolved

numpy/core/_add_newdocs.py Outdated Show resolved Hide resolved

numpy/core/tests/test_numeric.py Outdated Show resolved Hide resolved

seberg added this to the 1.17.0 release milestone Jun 12, 2019

seberg added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Jun 12, 2019

mhvk force-pushed the disallow-can-cast-of-everything-to-structured branch from a6f1713 to e12c6bd Compare June 12, 2019 18:48

seberg reviewed Jun 12, 2019

View reviewed changes

mhvk merged commit 6420e7f into numpy:master Jun 12, 2019

mhvk deleted the disallow-can-cast-of-everything-to-structured branch June 12, 2019 20:12

mhvk mentioned this pull request Jun 14, 2019

Errors with numpy dev: TypeError: Cannot cast array from dtype astropy/astropy#8841

Closed

effigies mentioned this pull request Jun 15, 2019

FIX/TEST: Numpy casting rules have gotten more strict, raise different exception nipy/nibabel#768

Merged

charris modified the milestones: 1.17.0 release, 1.16.5 release Jul 14, 2019

charris removed 09 - Backport-Candidate PRs tagged should be backported 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes labels Jul 17, 2019

charris mentioned this pull request Jul 17, 2019

BUG: ensure that casting to/from structured is properly checked. #14044

Merged

ahaldane mentioned this pull request Feb 4, 2020

BUG: Disallow type-promotion of dtypes with different field names #15509

Closed

BUG: ensure that casting to/from structured is properly checked. #13648

BUG: ensure that casting to/from structured is properly checked. #13648

Conversation

mhvk commented May 28, 2019 • edited Loading

eric-wieser May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk commented May 29, 2019

seberg left a comment

Choose a reason for hiding this comment

seberg left a comment

Choose a reason for hiding this comment

mhvk commented May 29, 2019

Current behaviour

mhvk commented May 29, 2019 • edited Loading

mhvk commented May 29, 2019

seberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser May 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk commented May 29, 2019

mhvk commented May 31, 2019

charris commented Jun 3, 2019

mhvk commented Jun 12, 2019

seberg commented Jun 12, 2019

mhvk commented Jun 12, 2019

charris commented Jun 12, 2019

seberg left a comment

Choose a reason for hiding this comment

mhvk commented Jun 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg commented Jun 12, 2019

mhvk commented May 28, 2019 •

edited

Loading

eric-wieser May 28, 2019 •

edited

Loading

mhvk commented May 29, 2019 •

edited

Loading

eric-wieser May 31, 2019 •

edited

Loading