10000 COMPAT: PyPy calls clongdouble_int which raises a warning by mattip · Pull Request #9181 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

COMPAT: PyPy calls clongdouble_int which raises a warning #9181

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
Merged
merged 1 commit into from
May 30, 2017
Merged

COMPAT: PyPy calls clongdouble_int which raises a warning #9181

merged 1 commit into from
May 30, 2017

Conversation

mattip
Copy link
Member
@mattip mattip commented May 27, 2017

Python calls x.tp_as_number.nb_int for either int(x) or x.__int__(). However, if that function pointer is modified after PyType_Ready, CPython will call the function registered when PyType_Ready is called for x.__int__(), but the new function for int(x).

In this case, the only difference between clongdouble_int (the new function switched in during add_scalarmath()) and clongdoubletype_int (the original function during PyType_Ready is that the latter one raises a Warning.

On PyPy we always call the new function, so tests (and live code) that use x.__int__() must accommodate for the warning. Code that calls int(x) will raise a warning on both platforms

@eric-wieser
Copy link
Member
eric-wieser commented May 27, 2017 •
edited
Loading

This feels like it's the __hash__ bug all over again (#8887).

However, if that function pointer is modified after PyType_Ready, CPython will call the function registered when PyType_Ready is called for x.int(), but the new function for int(x).

I would consider the fact that we modify the pointer after PyType_Ready a bug in numpy, just like it was last time. Is there a reason we can't fix this in the same way?

@mattip
Copy link
Member Author
mattip commented May 28, 2017

Note that after such a refactoring, the proposed patch would still be necessary, since now the call would raise a warning on both cpython and pypy :)

The code in question is in src/umath/scalarmath.c.src, so to refactor it would mean moving all this to multiarray and calling before the call in multiarraymodule.c to setup_scalartypes. That is a large refactoring, worthy of a branch, since the code in question needs PyUFunc_clearfperr, PyUFunc_getfperr, PyUFunc_GetPyValues, PyUFunc_handlefperr, which apparently cannot be simply linked into both umath and multiarray modules due to some interned string npy_um_str_pyvals_name

@eric-wieser
Copy link
Member

Seems like this test should just be calling int anyway, rather than invoking magic methods directly.

In this case, the only difference between clongdouble_int (the new function switched in during add_scalarmath()) and clongdoubletype_int (the original function during PyType_Ready is that the latter one raises a Warning.

Why does clongdoubletype_int exist at all?

@charris
Copy link
Member
charris commented May 28, 2017

There is consideration of moving the scalars back into the multiarray module, and maybe the rest of the ufunc machinery as well. That should be easier to do when npymath library contains all the needed functions and the scalars no longer need access to the ufuncs.

One topic that probably needs a discussion and decision is whether to keep the ufuncs separate. There are two contradictory ideas out there regarding ufuncs. One is to generalize ufuncs to operate on buffers, essentially separating them from their current entanglement with ndarrays. The other is to accept that they are fundamentally part of the ndarray universe and move them into the multiarray module, thus avoiding the odd overloading of functions in the multiarray module. The first has been a long time proposal that I thought sounded good in the beginning, but I'm coming around to a preference for the second. That change of mind is driven by the resulting code simplification and the removal of a dependence on a Python feature, buffers, that we cannot easily change to keep up with changing needs and new dtypes.
8000

@charris
Copy link
Member
charris commented May 28, 2017

Apropos the current PR, I agree wit @eric-wieser that the ideal solution is to use the correct function in the first place. The __int__ method in the type dictionary, as opposed to the slot function, is set up by PyType_Ready, so I suppose one could override that one also, but fooling with the slot wrappers is probably not worth the effort. I think keeping the warning is desirable, so the current test fix may be correct in the long term.

@njsmith
Copy link
Member
njsmith commented May 28, 2017

One is to generalize ufuncs to operate on buffers, essentially separating them from their current entanglement with ndarrays. The other is to accept that they are fundamentally part of the ndarray universe and move them into the multiarray module, thus avoiding the odd overloading of functions in the multiarray module.

Even if we were going to go with the first idea, then I still think it would make sense to merge the two modules on disk; they could be logically separated at the API level and still share implementation details and be shipped together. (Of course I actually prefer the second idea anyway, because it seems to me that the main things users want are new dtypes and new storage strategies, and neither of those fit into the buffer paradigm.)

@charris
Copy link
Member
charris commented May 29, 2017

Note that the failed test is a check for an "ignore" filter. You should use suppress_warnings instead. That is to deal with a bug in older versions of Python. Grep to find examples.

@mattip
Copy link
Member Author
mattip commented May 29, 2017

reworked, thanks for the comments. Not sure why it is showing commits other than b328445

@charris
Copy link
Member
charris commented May 29, 2017

Not sure why it is showing commits other than b328445

I expect you merged the master branch. You can probably fix it by rebasing on master and doing a force push.

@eric-wieser
Copy link
Member

Can we also add an @dec.knownFailure or whatever it's called that expects __int__ to raise the warning because int does?

eric-wieser
eric-wieser approved these changes May 30, 2017
View reviewed changes
Copy link
Member
@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

LGTM!

@charris charris merged commit 39117c5 into numpy:master May 30, 2017
@charris
Copy link
Member
charris commented May 30, 2017

Thanks @mattip .

@eric-wieser
Copy link
Member
eric-wieser commented Nov 6, 2017 •
edited
Loading

This is now causing me problems again in fixing #9964 in #9971.

It's really annoying that int(scalar) and scalar.__int__ mean different things. I've opened #9972 so that we don't forget

@eric-wieser eric-wieser mentioned this pull request Nov 6, 2017
Closed
@ghost ghost mentioned this pull request Dec 13, 2017
Merged
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 approved these changes

Assignees
No one assigned
Labels
05 - Testing 24 - PyPy component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mattip @eric-wieser @charris @njsmith
0