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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions doc/release/1.7.0-notes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 6 additions & 0 deletions doc/source/reference/ufuncs.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions numpy/core/code_generators/numpy_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
11 changes: 7 additions & 4 deletions numpy/core/include/numpy/ndarraytypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
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
 */

} NPY_CASTING;

typedef enum {
NPY_CLIP=0,
Expand Down
16 changes: 16 additions & 0 deletions numpy/core/src/multiarray/common.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.)
*/
Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Expand Down
31 changes: 31 additions & 0 deletions numpy/core/src/multiarray/convert_datatype.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
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.

{
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 */
Expand Down
18 changes: 18 additions & 0 deletions numpy/core/tests/test_ufunc.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
6 changes: 5 additions & 1 deletion numpy/testing/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
40 changes: 37 additions & 3 deletions numpy/testing/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -1464,7 +1465,7 @@ def assert_warns(warning_class, func, *args, **kw):

Returns
-------
None
The value returned by `func`.

"""

Expand All @@ -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__)
Expand All @@ -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