Skip to content

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

Merged
merged 4 commits into from
Sep 24, 2012
Merged

Conversation

njsmith
Copy link
Member

@njsmith njsmith commented Sep 20, 2012

  • Improve test functions for warning code
  • Switch back to "unsafe" as a default for ufunc out= arguments, but raise a DeprecationWarning on anything that violates the "same_kind" casting rule, so that we can switch to "same_kind" in a future release.

See commit messages for details.

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,
Copy link
Member

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
 */

@charris
Copy link
Member

charris commented Sep 20, 2012

Looks good apart from my world renowned style nitpicks. Thanks for taking care of this.

@njsmith
Copy link
Member Author

njsmith commented Sep 20, 2012

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
Copy link
Member

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..."

Copy link
Member Author

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)
Copy link
Member

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.

Copy link
Member Author

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.

@njsmith
Copy link
Member Author

njsmith commented Sep 21, 2012

The test failure is just the "invalid value in power" thing, everything else passes.

@charris
Copy link
Member

charris commented Sep 21, 2012

@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.

@njsmith
Copy link
Member Author

njsmith commented Sep 24, 2012

I'll just merge this for now and open a ticket for all the 1.7 deprecations so they don't get lost.

njsmith added a commit that referenced this pull request Sep 24, 2012
@njsmith njsmith merged commit c8010d0 into numpy:master Sep 24, 2012
@certik certik mentioned this pull request Sep 30, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants