BUG: Allow `int` to be called on nested object arrays, fix `np.str_.int` #10042

eric-wieser · 2017-11-17T07:23:49Z

Rather than assuming that any object array is self-referencing, we can just use PyEnter_RecursiveCall, in:

__int__
__float__
__long__
__hex__
__oct__

This works towards (but does not fix) #9972, by not trying to touch the nb_* slots ourselves (the same way as #10040).

Substantial code deduplication here. Error message is different, but perhaps also better:

>>> int(np.array([None]))
TypeError: int() argument must be a string, a bytes-like object or a number, not 'NoneType' # now
TypeError: cannot convert to an int; scalar object is not a number #before

mattip · 2017-11-17T07:37:15Z

numpy/core/src/multiarray/number.c

-                "object array may be self-referencing");
-        Py_DECREF(pv);
+    res = PyObject_CallMethodObjArgs(mod, "oct", o, NULL);
+    Py_DECREF(module);


"module" is not defined, you probably meant "mod"

Good catch, only tested on python 3 locally

mattip · 2017-11-17T07:37:40Z

numpy/core/src/multiarray/number.c

-    Py_DECREF(pv);
-    return pv2;
+    res = PyObject_CallMethodObjArgs(mod, "hex", o, NULL);
+    Py_DECREF(module);


"module" -> "mod" ?

mattip · 2017-11-17T07:45:37Z

numpy/core/tests/test_regression.py


        # Test the same for a circular reference.
        b = np.array(a, dtype=object)
        a[()] = b
-        assert_raises(TypeError, int, a)
+        assert_raises(RecursionError, int, a)


RecursionError is a python3-ism, is there a trick for python 2?

test_multiarray does:

try: Error = RecursionError except NameError: Error = RuntimeError # python < 3.5 assert_raises(Error, bool, self_containing) # previously stack overflow

Turns out it was me that wrote the code in multiarray... Again, good catch!

eric-wieser · 2017-11-17T08:05:25Z

Something fishy going on here on py 2... EDIT: PyCallMethodObjArgs didn't have the signature I thought it did. Solved.

mattip · 2017-11-17T08:12:55Z

document in release notes?

eric-wieser · 2017-11-17T08:17:03Z

Not sure it's note-worthy, since this is a bugfix for a bunch of contrived corner-cases. I don't think it has any compatibility ramifications either.

eric-wieser · 2017-11-17T08:19:59Z

numpy/core/src/multiarray/number.c

-array_int(PyArrayObject *v)
+array_scalar_forward(PyArrayObject *v,
+                     PyObject *(*builtin_func)(PyObject *),
+                     const char *where)


In principle we could use templating instead of function pointers, but we don't really care about speed here.

eric-wieser · 2017-11-17T09:14:30Z

Travis fails with:

numpy/core/src/multiarray/number.c: In function ‘array_scalar_forward’:
numpy/core/src/multiarray/number.c:842:9: warning: passing argument 1 of ‘_Py_CheckRecursiveCall’ discards ‘const’ qualifier from pointer target type [enabled by default]
         if (Py_EnterRecursiveCall(where) != 0) {
         ^
In file included from /usr/include/python3.4dm/Python.h:115:0,
                 from numpy/core/src/multiarray/number.c:2:
/usr/include/python3.4dm/ceval.h:80:17: note: expected ‘char *’ but argument is of type ‘const char *’
 PyAPI_FUNC(int) _Py_CheckRecursiveCall(char *where);
                 ^

However, this is a python bug, not a numpy bug - it's fixed in python/cpython@5fa22fc (3.4.4) and python/cpython@1670af6 (2.7.11), which are greater than or equal to the versions the python docs are generated from.

mattip · 2017-11-17T09:50:45Z

could you either skip the test or do a cast (for specifically that version)?

mattip · 2017-11-17T13:54:54Z

@eric-wieser This pull request is required for PyPy to pass the test suite. There is a regression somewhere from 1.13 wrt handling of string scalars, I am not sure exactly where. Could we mark the pull request for inclusion in 1.14?

eric-wieser · 2017-11-17T16:42:08Z

If this truly is a regression, it should make 1.13.4 - but I'm surprised, as I don't remember changes that would cause this - indeed, the tests of string_.__int__ fail on 1.12 for me.

eric-wieser · 2017-11-17T16:53:00Z

Milestone added, along with workaround to squash the warning for old python

Rather than assuming that any object array is self-referencing, we can just use PyEnter_RecursiveCall in: * `__int__` * `__float__` * `__long__` * `__hex__` * `__oct__` This works towards (but does _not_ fix) numpy#9972, by not directly touching the `nb_*` slots ourselves. Substantial code deduplication here. Error message is different, but perhaps also better: ```python >>> int(np.array([None])) TypeError: int() argument must be a string, a bytes-like object or a number, not 'NoneType' # now TypeError: cannot convert to an int; scalar object is not a number #before ```

ahaldane · 2017-11-18T00:49:32Z

Seems like a good cleanup, code LGTM. So it now it allows object arrays (that don't recurse), and uses the python-builtin conversion funcs instead of (badly) trying to do the conversion ourselves.

I don't totally see why this supersedes #10040 - that PR did adjustments of the tp_ slots. I presume we want to come back to the tp_ slot issues later. But on it's own this PR looks good.

eric-wieser · 2017-11-18T01:19:30Z

#10040 was needed specifically because we did a bad job of doing the conversion ourselves - our approach didn't special-case string types like the builtin PyNumber_Long does. Now that we use PyNumber_Long directly, everything is fine.

It does seem a little weird that str.__int__ does not exist in CPython...

mattip · 2017-11-18T18:52:26Z

@eric-wieser wrt regression comment above, for PyPy tests started failing after commit 931758e (#9856), "BUG: Make bool(void_scalar) and void_scalar.astype(bool) consistent", which added extra calls to PyArray_Scalar in @from@_to_@to@ conversions. After this pull request those tests again pass

And yes, it is strange that str.__int__ does not exists but int(str) works

eric-wieser · 2017-11-18T19:25:12Z

Thanks for the info - looks like I'm to blame for the accidental breakage, so it's fitting that I should accidentally fix it too.
Any idea why that change caused PyPy to fail? Can you point me to the currently failing test on 1.13?

mattip · 2017-11-18T19:43:49Z

That commit AFAICT is not part of a 1.13 release, as far as git tag --contains 931758e2 and git branch --contains 931758e2 can tell me

These tests newly fail after that commit (since in PyPy the logic in PyInt_FromString uses type.__int__ which reaches gentype_int which fails):

numpy.ma.tests.test_mrecords.TestMRecordsImport.test_fromarrays, raises TypeError: cannot convert to a float; scalar object is not a number
`numpy.ma.tests.test_core.TestFillingValues.test_fillvalue_conversionraisesTypeError: cannot convert to an int; scalar object is not a number``
numpy.core.tests.test_item_selection.TestTake.test_simple raises ``TypeError: cannot convert to an int; scalar object is not a number''
numpy.core.tests.test_indexing.TestFancyIndexingEquivalence.test_cast_equivalence raises TypeError: cannot convert to an int; scalar object is not a number

After this pull request those tests now pass

eric-wieser · 2017-11-18T19:50:23Z

Got it - the problem is that before in conversions we were doing int(arr.item()), and after #9856 we're doing int(arr[()]). The incentive was I suppose to avoid calling void.item(), which is broken until we work out how to safely merge #8157.

You're right, it's not in 1.13, so this only needs to make it into 1.14 for there to be no regression.

ahaldane · 2017-11-22T17:52:43Z

I think this is ready to go. I'll merge in a little while if there are no further comments.

ahaldane · 2017-11-22T23:54:11Z

Merging. Thanks Eric!

eric-wieser added the 00 - Bug label Nov 17, 2017

eric-wieser changed the title ~~BUG: Allow int to be called on nested object arrays~~ BUG: Allow int to be called on nested object arrays, fix np.str_.__int__ Nov 17, 2017

eric-wieser mentioned this pull request Nov 17, 2017

BUG: number.__int__ and int(number) are not the same #9972

Closed

mattip reviewed Nov 17, 2017

View reviewed changes

eric-wieser force-pushed the better-ndarray.__int__ branch from 93a0254 to 413c38a Compare November 17, 2017 07:55

mattip mentioned this pull request Nov 17, 2017

BUG: add nb_* conversions to fix issue #9972 #10040

Closed

eric-wieser force-pushed the better-ndarray.__int__ branch 2 times, most recently from b353de3 to a4c30ee Compare November 17, 2017 07:57

eric-wieser force-pushed the better-ndarray.__int__ branch from a4c30ee to a4c44aa Compare November 17, 2017 08:10

eric-wieser added the component: numpy._core label Nov 17, 2017

eric-wieser commented Nov 17, 2017

View reviewed changes

eric-wieser force-pushed the better-ndarray.__int__ branch 5 times, most recently from f6b7eda to 6c6da73 Compare November 17, 2017 08:49

eric-wieser force-pushed the better-ndarray.__int__ branch from 6c6da73 to 1578dda Compare November 17, 2017 16:51

eric-wieser modified the milestones: 1.13.4 release, 1.14.0 release Nov 17, 2017

eric-wieser force-pushed the better-ndarray.__int__ branch from 1578dda to bcec12b Compare November 17, 2017 17:00

eric-wieser mentioned this pull request Nov 19, 2017

CI: add pypy #10051

Closed

ahaldane merged commit 75b2d5d into numpy:master Nov 22, 2017

eric-wieser mentioned this pull request Mar 5, 2018

segfault with extension dtype not defining nb_int #10693

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Allow `int` to be called on nested object arrays, fix `np.str_.int` #10042

BUG: Allow `int` to be called on nested object arrays, fix `np.str_.int` #10042

eric-wieser commented Nov 17, 2017 •

edited

Loading

mattip Nov 17, 2017

eric-wieser Nov 17, 2017

mattip Nov 17, 2017

mattip Nov 17, 2017

mattip Nov 17, 2017

eric-wieser Nov 17, 2017 •

edited

Loading

eric-wieser commented Nov 17, 2017 •

edited

Loading

mattip commented Nov 17, 2017

eric-wieser commented Nov 17, 2017

eric-wieser Nov 17, 2017

eric-wieser commented Nov 17, 2017 •

edited

Loading

mattip commented Nov 17, 2017

mattip commented Nov 17, 2017

eric-wieser commented Nov 17, 2017 •

edited

Loading

eric-wieser commented Nov 17, 2017

ahaldane commented Nov 18, 2017

eric-wieser commented Nov 18, 2017 •

edited

Loading

mattip commented Nov 18, 2017 •

edited by eric-wieser

Loading

eric-wieser commented Nov 18, 2017 •

edited

Loading

mattip commented Nov 18, 2017

eric-wieser commented Nov 18, 2017 •

edited

Loading

ahaldane commented Nov 22, 2017

ahaldane commented Nov 22, 2017

BUG: Allow int to be called on nested object arrays, fix np.str_.__int__ #10042

BUG: Allow int to be called on nested object arrays, fix np.str_.__int__ #10042

Conversation

eric-wieser commented Nov 17, 2017 • edited Loading

mattip Nov 17, 2017

Choose a reason for hiding this comment

eric-wieser Nov 17, 2017

Choose a reason for hiding this comment

mattip Nov 17, 2017

Choose a reason for hiding this comment

mattip Nov 17, 2017

Choose a reason for hiding this comment

mattip Nov 17, 2017

Choose a reason for hiding this comment

eric-wieser Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

eric-wieser commented Nov 17, 2017 • edited Loading

mattip commented Nov 17, 2017

eric-wieser commented Nov 17, 2017

eric-wieser Nov 17, 2017

Choose a reason for hiding this comment

eric-wieser commented Nov 17, 2017 • edited Loading

mattip commented Nov 17, 2017

mattip commented Nov 17, 2017

eric-wieser commented Nov 17, 2017 • edited Loading

eric-wieser commented Nov 17, 2017

ahaldane commented Nov 18, 2017

eric-wieser commented Nov 18, 2017 • edited Loading

mattip commented Nov 18, 2017 • edited by eric-wieser Loading

eric-wieser commented Nov 18, 2017 • edited Loading

mattip commented Nov 18, 2017

eric-wieser commented Nov 18, 2017 • edited Loading

ahaldane commented Nov 22, 2017

ahaldane commented Nov 22, 2017

BUG: Allow `int` to be called on nested object arrays, fix `np.str_.int` #10042

BUG: Allow `int` to be called on nested object arrays, fix `np.str_.int` #10042

eric-wieser commented Nov 17, 2017 •

edited

Loading

eric-wieser Nov 17, 2017 •

edited

Loading

eric-wieser commented Nov 17, 2017 •

edited

Loading

eric-wieser commented Nov 17, 2017 •

edited

Loading

eric-wieser commented Nov 17, 2017 •

edited

Loading

eric-wieser commented Nov 18, 2017 •

edited

Loading

mattip commented Nov 18, 2017 •

edited by eric-wieser

Loading

eric-wieser commented Nov 18, 2017 •

edited

Loading

eric-wieser commented Nov 18, 2017 •

edited

Loading