-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
Unsafe cast deprecation #451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
4708615
cea0a20
c038fe5
f18987a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
In numpy 1.6 and earlier, if you do np.add(int_arr, float_arr, out=int_arr) or int_arr += float_arr then the result will be silently truncated to integer values. This often produces bugs, because it's easy to accidentally end up with an integer array and not realize it. Therefore, there seems to be consensus that we should switch to using same_kind casting by default for in-place ufunc operations. However, just switching this (as was done initially during the 1.7 development cycle) breaks a lot of code, which is rude and violates our deprecation policy. This commit instead adds a special temporary casting rule which acts like "unsafe", but also checks whether each operation would be allowed under "same_kind" rules and issues a DeprecationWarning if not. It also moves NPY_DEFAULT_ASSIGN_CASTING into the formal API instead of leaving it as a #define. This way we can change it later, and any code which references it and is compiled against this version of numpy will automatically switch to whatever we change it too. This avoids the situation where we want to remove the temporary magic value we're using to create DeprecationWarnings now, but can't because it would be an ABI break.
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,20 @@ | |
#include "common.h" | ||
#include "buffer.h" | ||
|
||
/* The casting to use for implicit assignment operations resulting from | ||
* in-place operations (like +=) and out= arguments. (Notice that this | ||
* variable is misnamed, but it's part of the public API so I'm not sure we | ||
* can just change it. Maybe someone should try and see if anyone notices. | ||
*/ | ||
/* In numpy 1.6 and earlier, this was NPY_UNSAFE_CASTING. In a future | ||
* release, it will become NPY_SAME_KIND_CASTING. Right now, during the | ||
* transitional period, we continue to follow the NPY_UNSAFE_CASTING rules (to | ||
* avoid breaking people's code), but we also check for whether the cast would | ||
* be allowed under the NPY_SAME_KIND_CASTING rules, and if not we issue a | ||
* warning (that people's code will be broken in a future release.) | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First line of multiline comments should be blank |
||
NPY_NO_EXPORT NPY_CASTING NPY_DEFAULT_ASSIGN_CASTING = NPY_INTERNAL_UNSAFE_CASTING_BUT_WARN_UNLESS_SAME_KIND; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will static do instead of NPY_NO_EXPORT ? It doesn't look like this is in a header. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I'm pretty sure that would break separate compilation -- it's declared extern in __multiarray_api.h, and references in other .c files need to be resolved to this definition by the linker. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it should be declared extern in a header? I don't know if the is compatible with NPY_NO_EXORT though, I think that becomes static in one file builds. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, the trick is that for one-file builds it has to be declared static, in multi-file builds it has to be declared extern in a header, and in builds of third-party packages it needs to be looked up via the famous array-of-pointers API structure. I'm pretty sure declaring it NPY_NO_EXPORT in a .c file plus putting it in numpy_api.py is the right way to automatically achieve all of those things. I found it useful to look at the generated __multiarray_api.h to see exactly what was going on. |
||
|
||
|
||
NPY_NO_EXPORT PyArray_Descr * | ||
_array_find_python_scalar_type(PyObject *op) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -503,12 +503,41 @@ type_num_unsigned_to_signed(int type_num) | |
} | ||
} | ||
|
||
/* NOTE: once the UNSAFE_CASTING -> SAME_KIND_CASTING transition is over, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blank line ;) |
||
* we should remove NPY_INTERNAL_UNSAFE_CASTING_BUT_WARN_UNLESS_SAME_KIND | ||
* and PyArray_CanCastTypeTo_impl should be renamed back to | ||
* PyArray_CanCastTypeTo. | ||
*/ | ||
static npy_bool | ||
PyArray_CanCastTypeTo_impl(PyArray_Descr *from, PyArray_Descr *to, | ||
NPY_CASTING casting); | ||
|
||
/*NUMPY_API | ||
* Returns true if data of type 'from' may be cast to data of type | ||
* 'to' according to the rule 'casting'. | ||
*/ | ||
NPY_NO_EXPORT npy_bool | ||
PyArray_CanCastTypeTo(PyArray_Descr *from, PyArray_Descr *to, | ||
NPY_CASTING casting) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit concerned because the GIL now needs to be held in case of the deprecation message. We should really be returning an int instead of npy_bool, but it is probably too late to change that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PyArray_CanCastTypeTo already requires the GIL -- it allocates and DECREFs dtype objects. |
||
{ | ||
if (casting == NPY_INTERNAL_UNSAFE_CASTING_BUT_WARN_UNLESS_SAME_KIND) { | ||
npy_bool unsafe_ok, same_kind_ok; | ||
unsafe_ok = PyArray_CanCastTypeTo_impl(from, to, NPY_UNSAFE_CASTING); | ||
same_kind_ok = PyArray_CanCastTypeTo_impl(from, to, | ||
NPY_SAME_KIND_CASTING); | ||
if (unsafe_ok && !same_kind_ok) { | ||
DEPRECATE("Implicitly casting between incompatible kinds. In " | ||
"a future numpy release, this will become an error. " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "raise" instead of "become" ? |
||
"Use casting=\"unsafe\" if this is intentional."); | ||
} | ||
return unsafe_ok; | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
return PyArray_CanCastTypeTo_impl(from, to, casting); | ||
} | ||
} | ||
|
||
static npy_bool | ||
PyArray_CanCastTypeTo_impl(PyArray_Descr *from, PyArray_Descr *to, | ||
NPY_CASTING casting) | ||
{ | ||
/* If unsafe casts are allowed */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -742,5 +742,23 @@ def t(expect, func, n, m): | |
uf.accumulate(np.zeros((30, 30)), axis=0) | ||
uf.accumulate(np.zeros((0, 0)), axis=0) | ||
|
||
def test_safe_casting(self): | ||
# In old numpy's, any casting was allowed for in-place operations. In | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In numpy versions <= 1.6.x unsafe casting was allowed for in-place operations... It looks like assignments still work, I wonder if that is an error?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eh, the unsafe casting rule is literally "return True", so the current text is actually a pretty accurate description :-). But yeah, fixed. Re: assignments: Yeah, I don't know, apparently assignments and NPY_DEFAULT_ASSIGN_CASTING have nothing to do with each other -- see my post on the list. And I guess the list is a better place to discuss any changes there anyway. |
||
# future numpy's, only same_kind casting will be allowed by | ||
# default. | ||
a = np.array([1, 2, 3], dtype=int) | ||
# Non-in-place addition is fine | ||
assert_array_equal(assert_no_warnings(np.add, a, 1.1), | ||
[2.1, 3.1, 4.1]) | ||
assert_warns(DeprecationWarning, np.add, a, 1.1, out=a) | ||
assert_array_equal(a, [2, 3, 4]) | ||
def add_inplace(a, b): | ||
a += b | ||
assert_warns(DeprecationWarning, add_inplace, a, 1.1) | ||
assert_array_equal(a, [3, 4, 5]) | ||
# Make sure that explicitly overriding the warning is allowed: | ||
assert_no_warnings(np.add, a, 1.1, out=a, casting="unsafe") | ||
assert_array_equal(a, [4, 5, 6]) | ||
|
||
if __name__ == "__main__": | ||
run_module_suite() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe NPY_UNSAFE_CASTING_WARN ?
Also the comment formatting should be