Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Unsafe cast deprecation #451

Merged
merged 4 commits into from

2 participants

@njsmith
Owner
  • 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.

njsmith added some commits
@njsmith njsmith ENH: More capable test functions for warnings
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.
4708615
@njsmith njsmith FIX: Transition scheme for safer in-place ufunc operations
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.
cea0a20
numpy/core/include/numpy/ndarraytypes.h
((7 lines not shown))
-/* The default casting to use for typical assignment operations */
-#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,
@charris Owner
charris added a note

Maybe NPY_UNSAFE_CASTING_WARN ?

Also the comment formatting should be

/*
 * blah
 */
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charris charris commented on the diff
numpy/core/src/multiarray/common.c
@@ -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.)
+ */
@charris Owner
charris added a note

First line of multiline comments should be blank

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charris charris commented on the diff
numpy/core/src/multiarray/common.c
@@ -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.)
+ */
+NPY_NO_EXPORT NPY_CASTING NPY_DEFAULT_ASSIGN_CASTING = NPY_INTERNAL_UNSAFE_CASTING_BUT_WARN_UNLESS_SAME_KIND;
@charris Owner
charris added a note

Will static do instead of NPY_NO_EXPORT ? It doesn't look like this is in a header.

@njsmith Owner
njsmith added a note

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.

@charris Owner
charris added a note

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.

@njsmith Owner
njsmith added a note

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
numpy/core/src/multiarray/convert_datatype.c
@@ -503,12 +503,41 @@
}
}
+/* NOTE: once the UNSAFE_CASTING -> SAME_KIND_CASTING transition is over,
@charris Owner
charris added a note

Blank line ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
numpy/core/src/multiarray/convert_datatype.c
((13 lines not shown))
/*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)
+{
+ 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. "
+ "Use casting=\"unsafe\" if this is intentional.");
+ }
+ return unsafe_ok;
+ } else {
@charris Owner
charris added a note
{
else {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
numpy/core/src/multiarray/convert_datatype.c
((13 lines not shown))
/*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)
+{
+ 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. "
@charris Owner
charris added a note

"raise" instead of "become" ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
numpy/core/tests/test_ufunc.py
@@ -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
@charris Owner
charris added a note

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?

In [1]: a = ones(3, int16)

In [2]: a[...] = 1.2

In [3]: a
Out[3]: array([1, 1, 1], dtype=int16)
@njsmith Owner
njsmith added a note

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.

@charris Owner
charris added a note

Numpy's is possessive, not plural, that's why I changed the wording. Maybe "In older versions of numpy..."

@njsmith Owner
njsmith added a note

Ah, gotcha. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charris
Owner

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

@njsmith
Owner

Fixed all the style issues. Thanks for the review!

@charris charris commented on the diff
numpy/core/src/multiarray/convert_datatype.c
((14 lines not shown))
/*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)
@charris Owner
charris added a note

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.

@njsmith Owner
njsmith added a note

PyArray_CanCastTypeTo already requires the GIL -- it allocates and DECREFs dtype objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njsmith
Owner

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

@charris
Owner

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

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

@njsmith njsmith merged commit c8010d0 into from
This was referenced
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 20, 2012
  1. @njsmith

    ENH: More capable test functions for warnings

    njsmith authored
    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.
  2. @njsmith

    FIX: Transition scheme for safer in-place ufunc operations

    njsmith authored
    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.
  3. @njsmith
  4. @njsmith
This page is out of date. Refresh to see the latest.
View
12 doc/release/1.7.0-notes.rst
@@ -33,10 +33,14 @@ np.diagonal, numpy 1.7 produces a FutureWarning if it detects
that you may be attemping to write to such an array. See the documentation
for array indexing for details.
-The default casting rule for UFunc out= parameters has been changed from
-'unsafe' to 'same_kind'. Most usages which violate the 'same_kind'
-rule are likely bugs, so this change may expose previously undetected
-errors in projects that depend on NumPy.
+In a future version of numpy, the default casting rule for UFunc out=
+parameters will be changed from 'unsafe' to 'same_kind'. (This also
+applies to in-place operations like a += b, which is equivalent to
+np.add(a, b, out=a).) Most usages which violate the 'same_kind' rule
+are likely bugs, so this change may expose previously undetected
+errors in projects that depend on NumPy. In this version of numpy,
+such usages will continue to succeed, but will raise a
+DeprecationWarning.
Full-array boolean indexing has been optimized to use a different,
optimized code path. This code path should produce the same results,
View
6 doc/source/reference/ufuncs.rst
@@ -309,6 +309,12 @@ advanced usage and will not typically be used.
'equiv', 'safe', 'same_kind', or 'unsafe'. See :func:`can_cast` for
explanations of the parameter values.
+ In a future version of numpy, this argument will default to
+ 'same_kind'. As part of this transition, starting in version 1.7,
+ ufuncs will produce a DeprecationWarning for calls which are
+ allowed under the 'unsafe' rules, but not under the 'same_kind'
+ rules.
+
*order*
.. versionadded:: 1.6
View
2  numpy/core/code_generators/numpy_api.py
@@ -14,10 +14,12 @@
multiarray_global_vars = {
'NPY_NUMUSERTYPES': 7,
+ 'NPY_DEFAULT_ASSIGN_CASTING': 292,
}
multiarray_global_vars_types = {
'NPY_NUMUSERTYPES': 'int',
+ 'NPY_DEFAULT_ASSIGN_CASTING': 'NPY_CASTING',
}
multiarray_scalar_bool_values = {
View
11 numpy/core/include/numpy/ndarraytypes.h
@@ -199,11 +199,14 @@ typedef enum {
/* Allow safe casts or casts within the same kind */
NPY_SAME_KIND_CASTING=3,
/* Allow any casts */
- NPY_UNSAFE_CASTING=4
-} NPY_CASTING;
+ NPY_UNSAFE_CASTING=4,
-/* The default casting to use for typical assignment operations */
-#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,
+} NPY_CASTING;
typedef enum {
NPY_CLIP=0,
View
16 numpy/core/src/multiarray/common.c
@@ -13,6 +13,22 @@
#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.)
+ */
@charris Owner
charris added a note

First line of multiline comments should be blank

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+NPY_NO_EXPORT NPY_CASTING NPY_DEFAULT_ASSIGN_CASTING = NPY_INTERNAL_UNSAFE_CASTING_BUT_WARN_UNLESS_SAME_KIND;
@charris Owner
charris added a note

Will static do instead of NPY_NO_EXPORT ? It doesn't look like this is in a header.

@njsmith Owner
njsmith added a note

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.

@charris Owner
charris added a note

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.

@njsmith Owner
njsmith added a note

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
NPY_NO_EXPORT PyArray_Descr *
_array_find_python_scalar_type(PyObject *op)
View
31 numpy/core/src/multiarray/convert_datatype.c
@@ -503,12 +503,43 @@ type_num_unsigned_to_signed(int type_num)
}
}
+/*
+ * NOTE: once the UNSAFE_CASTING -> SAME_KIND_CASTING transition is over,
+ * 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)
@charris Owner
charris added a note

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.

@njsmith Owner
njsmith added a note

PyArray_CanCastTypeTo already requires the GIL -- it allocates and DECREFs dtype objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+{
+ 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 raise an error. "
+ "Use casting=\"unsafe\" if this is intentional.");
+ }
+ return unsafe_ok;
+ }
+ else {
+ 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 */
View
18 numpy/core/tests/test_ufunc.py
@@ -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 versions of numpy, in-place operations used the 'unsafe'
+ # casting rules. In some future version, 'same_kind' will become the
+ # 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()
View
6 numpy/testing/tests/test_utils.py
@@ -317,11 +317,15 @@ class TestWarns(unittest.TestCase):
def test_warn(self):
def f():
warnings.warn("yo")
+ return 3
before_filters = sys.modules['warnings'].filters[:]
- assert_warns(UserWarning, f)
+ assert_equal(assert_warns(UserWarning, f), 3)
after_filters = sys.modules['warnings'].filters
+ assert_raises(AssertionError, assert_no_warnings, f)
+ assert_equal(assert_no_warnings(lambda x: x, 1), 1)
+
# Check that the warnings state is unchanged
assert_equal(before_filters, after_filters,
"assert_warns does not preserver warnings state")
View
40 numpy/testing/utils.py
@@ -16,7 +16,8 @@
'decorate_methods', 'jiffies', 'memusage', 'print_assert_equal',
'raises', 'rand', 'rundocs', 'runstring', 'verbose', 'measure',
'assert_', 'assert_array_almost_equal_nulp',
- 'assert_array_max_ulp', 'assert_warns', 'assert_allclose']
+ 'assert_array_max_ulp', 'assert_warns', 'assert_no_warnings',
+ 'assert_allclose']
verbose = 0
@@ -1464,7 +1465,7 @@ def assert_warns(warning_class, func, *args, **kw):
Returns
-------
- None
+ The value returned by `func`.
"""
@@ -1474,7 +1475,7 @@ def assert_warns(warning_class, func, *args, **kw):
l = ctx.__enter__()
warnings.simplefilter('always')
try:
- func(*args, **kw)
+ result = func(*args, **kw)
if not len(l) > 0:
raise AssertionError("No warning raised when calling %s"
% func.__name__)
@@ -1483,3 +1484,36 @@ def assert_warns(warning_class, func, *args, **kw):
"%s( is %s)" % (func.__name__, warning_class, l[0]))
finally:
ctx.__exit__()
+ return result
+
+def assert_no_warnings(func, *args, **kw):
+ """
+ Fail if the given callable produces any warnings.
+
+ Parameters
+ ----------
+ func : callable
+ The callable to test.
+ \\*args : Arguments
+ Arguments passed to `func`.
+ \\*\\*kwargs : Kwargs
+ Keyword arguments passed to `func`.
+
+ Returns
+ -------
+ The value returned by `func`.
+
+ """
+ # XXX: once we may depend on python >= 2.6, this can be replaced by the
+ # warnings module context manager.
+ ctx = WarningManager(record=True)
+ l = ctx.__enter__()
+ warnings.simplefilter('always')
+ try:
+ result = func(*args, **kw)
+ if len(l) > 0:
+ raise AssertionError("Got warnings when calling %s: %s"
+ % (func.__name__, l))
+ finally:
+ ctx.__exit__()
+ return result
Something went wrong with that request. Please try again.