Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

mattip
Copy link
Member

@mattip mattip commented Nov 16, 2017

string- and unicode- ndarrays followed different code paths for int(a) vs a.__int__() and float(a) vs. a.__float(). Implement a nb_int and nb_float to rectify this.

Unfortunately setting tp_as_number before calling PyType_Ready has the side-effect of filling all NULL nb_* slots with functions via the mro inheritance path (pulling them from PyGenericType_Type). String types should not have a nb_multiply nor a nb_add since they conflict with the sq_repeat and sq_concat functions from PyString_Type.

Fixes issue #9972

TODO: document

eric-wieser
eric-wieser reviewed Nov 17, 2017
View reviewed changes
numpy/core/src/multiarray/scalartypes.c.src
if (PyString_Type.tp_as_number)
stringtype_as_number = *PyString_Type.tp_as_number;
stringtype_as_number.nb_int = int_from_string;
stringtype_as_number.nb_float = float_from_string;
Copy link
Member

@eric-wieser eric-wieser Nov 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Why can't you just use PyString_Type.nb_float here? - hmm, I see it doesn't have one :(

>>> "".__float__
AttributeError: 'str' object has no attribute '__float__'
>>> "".__int__
AttributeError: 'str' object has no attribute '__int__'

Copy link
Member

Choose a reason for hiding this comment

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

Can you just use PyNumber_Int(PyObject_Str(self)) here instead (swapping str for unicode, and int for float as needed)`

@eric-wieser
Copy link
Member

eric-wieser commented Nov 17, 2017 •
edited
Loading

I don't follow your description of the problem. Do both __int__ and int follow the MRO, or just one of them?

@mattip
Copy link
Member Author

mattip commented Nov 17, 2017

Calling __int__ follows the MRO, looking for nb_int and finding the one on PyGenericArrType_Type (before this pull request).

Calling int(a) the chain is int_new(), which calls PyNumber_Long (PyNumber_Int on 2.7), which checks for nb_int on the current type object without traversing the MRO. If it exists, that is used, if not it eventually calls PyLong_FromString (PyInt_FromString).

Using PyNumber_Long as nb_int would cause infinite recursion.

@eric-wieser
Copy link
Member

I'm suggesting we call PyNumber_Long on the scalar, rather than trying to emulate PyNumber_Long like we currently do. Patch in the works.

@eric-wieser eric-wieser mentioned this pull request Nov 17, 2017
Merged
@mattip
Copy link
Member Author

mattip commented Nov 17, 2017

#10042 supersedes this, cleaner and more explicit

@mattip mattip closed this Nov 17, 2017
@mattip mattip deleted the scalar-nb_int branch November 17, 2017 07:56
@eric-wieser
Copy link
Member

Thanks for doing the diagnosis for me that lead to my patch!

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

@eric-wieser eric-wieser eric-wieser left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mattip @eric-wieser