ENH: expose cython isinstance checks for np.bool_, np.integer_, np.floating by jbrockmendel · Pull Request #16363 · numpy/numpy

jbrockmendel · 2020-05-24T18:37:28Z

Follows #16266 to upstream a few more cython-isinstance checks from pandas.

numpy/__init__.pxd

eric-wieser · 2020-05-24T19:19:37Z

numpy/__init__.pxd

Does cython not just let you run this directly?

it does, but it doesnt optimize it into a C call

Is the optimization even worth it? Isn't isinstance a thin wrapper around PyType_Check anyway?

Would it be worth filing a issue against cython requesting for it to optimize this type of thing automatically?

These just seem like weird helper functions to add to me, and I the only justification I can think of would be upstream deficiencies - in which case I'd like to see links to open cython issues in our source code.

(This complaint applies equally to https://github.com/numpy/numpy/pull/16266/files#r429937827 from the previous patch, although the rest of it seemed fine)

There is a difference, PyObject_TypeCheck does not support metaclasses/__subclasscheck__ or __subclasshook__, so for this type of function it may actually make a real difference.

... except that the function calls isinstance(obj, bool) in the first half

So to summarize so far:

this optimization is worth it, but maybe belongs upsrtream in Cython, which doesn't seem easy to do

there was a suggestion to put these into a namespace, which seems non-trivial

which leaves us with accepting these functions as work-arounds, much like the python work-arounds we have in other places. Perhaps we can mark them as such with some comments and accept this as-is?

this optimization is worth it, but maybe belongs upsrtream in Cython, which doesn't seem easy to do

not sure i understand this. My plan is that if/when this is merged, I'll make a PR with cython to make the same edits to their numpy/__init__.pxd file

These functions to two things: First they do some optimizations that cython should do in isinstance(), but fails to do. Second, they make writing isinstance(value, (python_type, numpy_type)) slight shorter. I think what Matti is saying that the second point may not be worth it. But the first point is worth it until cython is capable of parsing the full isinstance check into something faster (or maybe providing an isinstancefast/hastype check with that signature). Once Cython has such a new feature, these functions could probably be considered deprecated.

EDIT: To be clear, that is my current opinion as wel.

Cython does translate isinstance(value, (python_type, numpy_type)) into the corresponding C-API calls, except for bool, as @jbrockmendel pointed out. Calling isinstance(obj, (float, floating)) should be just fine.

numpy/__init__.pxd

numpy/core/tests/test_cython.py

jbrockmendel · 2020-06-06T16:08:07Z

CI failure looks like a mingw installation problem, presumably unrelated?

jbrockmendel · 2020-06-17T21:28:12Z

@mattip thoughts on how to get travis to green?

seberg · 2020-06-18T00:10:38Z

Don't worry about that particular run, its notorious. I restarted it, but chances are it will just fail again.

jbrockmendel · 2020-06-29T19:20:52Z

if we're not worrying about travis, anything else here thats actionable?

jbrockmendel · 2020-07-08T17:18:27Z

@seberg gentle ping

seberg · 2020-07-16T19:48:16Z

OK, so I think the main reason this has stalled, is that these functions are clearly useful and simple. But also feel a bit out of place in some sense (and I suppose even slightly annoying in case we ever feel like updating any of the definitions). Partially because they are so simple probably.

I am +0 to putting it in right now. A quick show of hands (or just note that nobody is against it), would be helpful!

mattip · 2020-07-17T05:25:59Z

I would be more positive on this if the docstrings referenced an issue in Cython. Or a one-sentence summary of the long discussion of the problem in the docstring and why Cython cannot solve this cleanly.

jbrockmendel · 2020-07-17T15:47:13Z

I would be more positive on this if the docstrings referenced an issue in Cython. Or a one-sentence summary of the long discussion of the problem in the docstring

Good idea, will update.

and why Cython cannot solve this cleanly

I'm not clear on what you're looking for here. If/when this is merged, I'll make a PR on cython porting the same functions to their numpy/init.pxd file, but my understanding is that that file is going away in favor of just having that live in numpy.

eric-wieser · 2020-07-17T15:53:41Z

and why Cython cannot solve this cleanly

I'm not clear on what you're looking for here.

Your docstrings say "Cython equivalent of isinstance(val, (float, np.float_))". The question is, "why isn't isinstance(val, (float, np.float_)) a good option in cython". The answer to this is "it does, but it doesnt optimize it into a C call".

So the follow up question is "why can't cython fix [optimizing it into a C call]?".

eric-wieser · 2020-07-17T15:55:24Z

numpy/__init__.pxd

Suggested change

Cython equivalent of `isinstance(val, (float, np.float_))`

Cython equivalent of `isinstance(val, (float, np.floating))`

eric-wieser · 2020-07-17T15:55:39Z

numpy/__init__.pxd

Suggested change

Cython equivalent of `isinstance(val, (complex, np.complex_))`

Cython equivalent of `isinstance(val, (complex, np.complexfloating))`

jbrockmendel · 2020-07-27T16:47:56Z

Updated per requests

eric-wieser · 2020-07-27T16:50:46Z

Cython cannot optimize this into a C call on its own because np.integer is not available as a C type.

Obvious follow-up question: how can we / why can't we make np.integer available as a C type?

jbrockmendel · 2020-07-27T17:01:45Z

Obvious follow-up question: how can we / why can't we make np.integer available as a C type?

I don't know/understand the numpy internals well enough to answer that. np.integer.__module__ shows "numpy", so it isnt clear where it is defined. Is it exposed in a .h file somewhere?

eric-wieser · 2020-07-27T17:05:39Z

When you say "exposed as a C type", do you mean

Cython is not told that np.integer and &PyIntegerArrType_Type are identical
Cython is not told the underlying storage format of np.integer (it has none)
Both of the above

eric-wieser · 2020-07-27T17:10:29Z

It sounds like all you need is

ctypedef class numpy.integer [object PyObject, type PyIntegerArrType_Type]:
    pass

and similar for the other types

eric-wieser · 2020-08-12T22:01:41Z

What happens if you also omit the type argument?

jbrockmendel · 2020-08-18T23:58:55Z

What happens if you also omit the type argument?

sorry, im not clear on what you have in mind. weve gone through a lot of iterations

scoder · 2020-08-23T07:20:30Z

What happens if you also omit the type argument?

sorry, im not clear on what you have in mind. weve gone through a lot of iterations

cdef extern from "numpy/scalartypes.h":
    ctypedef class numpy.floating [object PyObject]:
        pass

public exports things from your module, whereas extern imports/includes from other places, so both cannot go together. For an extern class, Cython executes a Python import at initialisation time to get at the 'real' type object (thus the need for the qualified module name). That conflicts with a C type name declaration (it thus rejects the type …). All it needs at compile time is the struct layout of the object, and that is just PyObject in this specific case.

…oating

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>

jbrockmendel · 2020-08-23T16:10:00Z

ctypedef class numpy.floating [object PyObject]:

Shouldn't PyFloatingArrType_Type be in here somewhere?

eric-wieser · 2020-08-23T16:13:54Z

Shouldn't PyFloatingArrType_Type be in here somewhere?

There's no reason for it to be. At worse cython pays a one-time cost of calling getattr to find the PyTypeObject* pointer, but after that there's no difference.

jbrockmendel · 2020-08-23T21:40:44Z

ctypedef class numpy.floating [object PyObject]:

no dice, see CI logs from most recent run.

At this point my feeling is that we have a working implementation and should go with that. If someone wants to try to figure out another solution more power to them, but it shouldn't be me.

eric-wieser · 2020-08-23T21:44:08Z

numpy/__init__.pxd

    /* NumPy API declarations from "numpy/__init__.pxd" */
    """

+cdef extern from "numpy/scalartypes.h":


CI failure is that this header doesn't exist.

Suggested change

cdef extern from "numpy/scalartypes.h":

cdef extern from *:

eric-wieser · 2020-08-23T22:19:42Z

If someone wants to try to figure out another solution more power to them, but it shouldn't be me.

That's fair. @scoder, would you have any interest in doing this? Otherwise, I can have a go ~~in two weeks~~ tomorrow.

scoder · 2020-08-24T09:03:47Z

The last change looks good to me, with the tweak by @eric-wieser. Could you apply that and let CI try again?

eric-wieser · 2020-08-24T09:10:59Z

Also worth remembering: before merging, whatever change we decide on needs to be duplicated to numpy/__init__.cython-30.pxd

eric-wieser · 2020-08-25T08:50:14Z

I went ahead and added definitions for all the abstract types in #17150. Assuming that gets merged, would you be interested in adding the concrete ones @jbrockmendel?

jbrockmendel · 2020-08-25T15:39:22Z

I went ahead and added definitions for all the abstract types in #17150. Assuming that gets merged, would you be interested in adding the concrete ones

Neat!

Maybe? I'm not sure what the "concrete ones" are in this context.

Since the CI says that ctypedef class numpy.floating [object PyObject]: works, I'm convinced but still confused. How does cython find that this corresponds to PyFloatingArrType_Type?

eric-wieser · 2020-08-25T15:45:14Z

How does cython find that this corresponds to PyFloatingArrType_Type?

Simple: it doesn't. cython emits code in the module startup like:

PyTypeObject *the_type_for_numpy_floating = PyObject_GetAttr(numpy_module, "floating");

As it turns out, the_type_for_numpy_floating == &PyFloatingArrType_Type; but cython doesn't need to know that, all it needs to know is that it can use the_type_for_numpy_floating wherever it sees cnp.floating.

eric-wieser · 2020-08-25T15:46:00Z

Maybe? I'm not sure what the "concrete ones" are in this context.

The ones you can actually create instances of, rather than the abstract base classes.

eric-wieser · 2020-09-13T08:18:19Z

Since #17150 is merged, are you happy to close this @jbrockmendel?

jbrockmendel · 2020-09-13T20:50:12Z

I think the is_integer_object check is worthwhile, as it is distinct from just the isinstance check.

I'll run a performance check on the others and see if it makes a difference.

eric-wieser · 2020-09-13T21:41:05Z

numpy/__init__.pxd

+    -----
+    This does not counts np.timedelta64 objects as integers.
+    """
+    return (not PyBool_Check(obj) and PyArray_IsIntegerScalar(obj)


Note the definition of this function is just:

numpy/numpy/core/include/numpy/ndarrayobject.h

Lines 51 to 52 in 6ee4917

#define PyArray_IsIntegerScalar(obj) (PyLong_Check(obj) \

|| PyArray_IsScalar((obj), Integer))

aka isinstance(obj, (int, np.integer))

yah, its excluding bool and td64 that makes this useful

Sure, but after that change it no longer uses any numpy C API functions, so it probably doesn't belong in numpy/__init__.pxd any more.

jbrockmendel · 2020-09-16T19:48:01Z

OK, ill close this and can revisit if performance discrepancies popup.

eric-wieser reviewed May 24, 2020

View reviewed changes

numpy/__init__.pxd Outdated Show resolved Hide resolved

eric-wieser reviewed May 24, 2020

View reviewed changes

charris added 01 - Enhancement component: build component: numpy._core labels May 24, 2020

eric-wieser reviewed May 25, 2020

View reviewed changes

numpy/__init__.pxd Outdated Show resolved Hide resolved

eric-wieser mentioned this pull request May 25, 2020

ENH: include dt64/td64 isinstance checks in __init__.pxd #16266

Merged

mattip reviewed May 31, 2020

View reviewed changes

numpy/core/tests/test_cython.py Outdated Show resolved Hide resolved

jbrockmendel force-pushed the cyapi2 branch from bbf21cf to f82f518 Compare June 5, 2020 18:06

jbrockmendel force-pushed the cyapi2 branch 2 times, most recently from d2bc708 to d83d59a Compare June 17, 2020 01:58

seberg self-requested a review July 8, 2020 17:27

seberg added the triage review Issue/PR to be discussed at the next triage meeting label Jul 16, 2020

eric-wieser reviewed Jul 17, 2020

View reviewed changes

jbrockmendel force-pushed the cyapi2 branch from d83d59a to 1942fbb Compare July 23, 2020 20:37

jbrockmendel force-pushed the cyapi2 branch from 2eb0ca9 to 0fc0d42 Compare August 18, 2020 23:57

jbrockmendel and others added 5 commits August 23, 2020 09:08

ENH: expose cython isinstance checks for np.bool_, np.integer_, np.fl…

1d6755a

…oating

Update numpy/__init__.pxd

d325e4c

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>

update per comments

0617553

fixup copy/paste screwup

aa9ac21

DOC: flesh out docstrings

c1ced44

try suggestion

8a9ce47

jbrockmendel force-pushed the cyapi2 branch from 0fc0d42 to 8a9ce47 Compare August 23, 2020 21:07

eric-wieser reviewed Aug 23, 2020

View reviewed changes

eric-wieser mentioned this pull request Aug 24, 2020

ENH: Add support for the abstract scalars to cython code #17150

Merged

try extern from star

35798f2

eric-wieser added the 57 - Close? Issues which may be closable unless discussion continued label Sep 13, 2020

eric-wieser reviewed Sep 13, 2020

View reviewed changes

jbrockmendel closed this Sep 16, 2020

	Cython equivalent of `isinstance(val, (float, np.float_))`
	Cython equivalent of `isinstance(val, (float, np.floating))`

	Cython equivalent of `isinstance(val, (complex, np.complex_))`
	Cython equivalent of `isinstance(val, (complex, np.complexfloating))`

	#define PyArray_IsIntegerScalar(obj) (PyLong_Check(obj) \
	\|\| PyArray_IsScalar((obj), Integer))

Uh oh!

Conversation

jbrockmendel commented May 24, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser May 25, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg Jun 18, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jbrockmendel commented Jun 6, 2020

Uh oh!

jbrockmendel commented Jun 17, 2020

Uh oh!

seberg commented Jun 18, 2020

Uh oh!

jbrockmendel commented Jun 29, 2020

Uh oh!

jbrockmendel commented Jul 8, 2020

Uh oh!

seberg commented Jul 16, 2020

Uh oh!

mattip commented Jul 17, 2020

Uh oh!

jbrockmendel commented Jul 17, 2020

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbrockmendel commented Jul 27, 2020

Uh oh!

eric-wieser commented Jul 27, 2020

Uh oh!

jbrockmendel commented Jul 27, 2020

Uh oh!

eric-wieser commented Jul 27, 2020

Uh oh!

eric-wieser commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbrockmendel commented Aug 18, 2020

Uh oh!

scoder commented Aug 23, 2020

Uh oh!

jbrockmendel commented Aug 23, 2020

Uh oh!

eric-wieser commented Aug 23, 2020

Uh oh!

jbrockmendel commented Aug 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser May 25, 2020 •

edited

Loading

seberg Jun 18, 2020 •

edited

Loading

eric-wieser commented Jul 17, 2020 •

edited

Loading

eric-wieser commented Jul 27, 2020 •

edited

Loading

eric-wieser commented Aug 12, 2020 •

edited

Loading

eric-wieser commented Aug 23, 2020 •

edited

Loading

eric-wieser commented Aug 24, 2020 •

edited

Loading

eric-wieser commented Aug 25, 2020 •

edited

Loading

eric-wieser Sep 13, 2020 •

edited

Loading