BUG: Allow `int` to be called on nested object arrays, fix `np.str_.__int__` by eric-wieser · Pull Request #10042 · numpy/numpy

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

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

Good catch, only tested on python 3 locally

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

"module" -> "mod" ?

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

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

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

Comment thread numpy/core/src/multiarray/number.c Outdated

mattip Nov 17, 2017

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"module" -> "mod" ?

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

Closed

Conversation

eric-wieser commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

eric-wieser Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

mattip Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

mattip Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

mattip Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

eric-wieser Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented Nov 17, 2017

Uh oh!

eric-wieser commented Nov 17, 2017

Uh oh!

eric-wieser Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented Nov 17, 2017

Uh oh!

mattip commented Nov 17, 2017

Uh oh!

eric-wieser commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented Nov 17, 2017

Uh oh!

ahaldane commented Nov 18, 2017

Uh oh!

eric-wieser commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented Nov 18, 2017 • edited by eric-wieser Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented Nov 18, 2017

Uh oh!

eric-wieser commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahaldane commented Nov 22, 2017

Uh oh!

ahaldane commented Nov 22, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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