-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Conversation
1) New function assert_no_warnings 2) Make assert_warns and assert_no_warnings pass through the function's return value on success, so that it can be checked as well.
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.
#define NPY_DEFAULT_ASSIGN_CASTING NPY_SAME_KIND_CASTING | ||
/* Temporary internal definition only, will be removed in upcoming | ||
release, see below */ | ||
NPY_INTERNAL_UNSAFE_CASTING_BUT_WARN_UNLESS_SAME_KIND = 100, |
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
/*
* blah
*/
Looks good apart from my world renowned style nitpicks. Thanks for taking care of this. |
Fixed all the style issues. Thanks for the review! |
@@ -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, unsafe casting was allowed for in-place |
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.
Numpy's is possessive, not plural, that's why I changed the wording. Maybe "In older versions of numpy..."
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.
Ah, gotcha. Fixed.
/*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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
PyArray_CanCastTypeTo already requires the GIL -- it allocates and DECREFs dtype objects.
The test failure is just the "invalid value in power" thing, everything else passes. |
@teoliphant @certik I think it can go in then. I'm less sure it should go into master as well as 1.7.x. Numpy 1.8 is where we were planning to be less concerned with backward compatibility. It's probably time to bring this up on the list again since Travis decided to push 1.8 planning off until after the release. |
I'll just merge this for now and open a ticket for all the 1.7 deprecations so they don't get lost. |
See commit messages for details.