[MRG] Make subfunctions of sparsefuncs private #6659

yenchenlin · 2016-04-14T12:06:03Z

In my previous merged PR #6539 ,
I forgot to use cdef for subfunctions, which may expose these subfunctions as part of the Python-level API.

This PR would fix the issue described above.

ping @jnothman @MechCoder @ogrisel

Thanks, and sorry for my carelessness.

raghavrv · 2016-04-14T12:10:29Z

sklearn/utils/sparsefuncs_fast.pyx

-def _inplace_csr_row_normalize_l1(np.ndarray[floating, ndim=1] X_data, shape,
+@cython.boundscheck(False)
+@cython.wraparound(False)
+@cython.cdivision(True)


Why do we need these?

(Sorry if that is obvious)

These are a large part of what makes Cython faster than Python.

Without those checks disabled, the expression X[i] (for int[:] X, int i) generates:

__pyx_t_1 = __pyx_v_i; __pyx_t_2 = -1; if (__pyx_t_1 < 0) { __pyx_t_1 += __pyx_v_X.shape[0]; if (unlikely(__pyx_t_1 < 0)) __pyx_t_2 = 0; } else if (unlikely(__pyx_t_1 >= __pyx_v_X.shape[0])) __pyx_t_2 = 0; if (unlikely(__pyx_t_2 != -1)) { __Pyx_RaiseBufferIndexError(__pyx_t_2); {__pyx_filename = __pyx_f[0]; __pyx_lineno = 2; __pyx_clineno = __LINE__; goto __pyx_L1_error;} } __pyx_r = (*((int *) ( /* dim=0 */ (__pyx_v_X.data + __pyx_t_1 * __pyx_v_X.strides[0]) )));

With the checks disabled, it's just the last line.

Thanks for @jnothman 's explanation, Cool!

@rvraghav93 I add it partially just because original code has these.
But for @cython.cdivision(True), you can see this

Thanks heaps for the explanation Joel!! As I always say, I learn a lot from your comments!

Thanks heaps for the explanation Joel!! As I always say, I learn a lot from your comments!

You're welcome. The intention with that comment was actually "Cython's not so scary. Write a three line snippet and try it yourself!" I hope it worked.

@rvraghav93 This is also documented here (http://docs.cython.org/src/reference/compilation.html#compiler-directives)

jnothman · 2016-04-14T12:21:55Z

LGTM

yenchenlin · 2016-04-14T12:24:37Z

@jnothman wait it broke ...
It seems that a function can not call a cdef subfunction?

raghavrv · 2016-04-14T12:28:12Z

It can break with MRG state. You can rename it back to [MRG+1]. Core devs won't merge unless the tests pass finally!

jnothman · 2016-04-14T12:28:39Z

sklearn/utils/sparsefuncs_fast.pyx

+@cython.boundscheck(False)
+@cython.wraparound(False)
+@cython.cdivision(True)
+cdef _inplace_csr_row_normalize_l1(np.ndarray[floating, ndim=1] X_data, shape,


actually, should be cdef void

jnothman · 2016-04-14T12:29:22Z

Sorry, I was a bit trigger-happy.

jnothman · 2016-04-14T12:30:56Z

Btw, "private" isn't really the correct description. The difference between def and cdef lies in whether the function needs to be exposed to Python meaning it needs to accept Python objects as input (and hence handle any argument list parsing and conversion) and return a Python object as output.

jnothman · 2016-04-14T12:34:17Z

sklearn/utils/sparsefuncs_fast.pyx

@@ -275,13 +275,16 @@ def incr_mean_variance_axis0(X, last_mean, last_var, unsigned long last_n):
 @cython.wraparound(False)


These flags are no longer relevant to this function

Though perhaps the flags should be applied at the module level...

@jnothman
Did you mean add

#!python #cython: wraparound=False

in front of any code?

Yes, that would prevent rewriting it again and again.

yenchenlin · 2016-04-14T16:17:16Z

hmm ... still broke.
I've tried to solve it but I still can't figure out what's wrong 😢

@jnothman any idea?

raghavrv · 2016-04-14T16:24:55Z

The cdef functions are those which can be called by other cdef and cpdef functions (those which will be compiled). You are attempting to call that from a python function def, which is why the cython compiler is complaining that there is "no suitable method found" (which can be accessed from inside a python function).

~~You need to declare them using cpdef to make them accessible from both C and python~~

http://docs.cython.org/src/reference/language_basics.html#functions-and-methods

Apologies for misunderstanding this :@ This is incorrect. Refer Joel's comment instead.

MechCoder · 2016-04-14T20:51:46Z

The difference between def and cdef lies in whether the function needs to be exposed to Python meaning it needs to accept Python objects as input.

Is that true? From here (http://docs.cython.org/src/reference/language_basics.html#functions-and-methods) it says both def and cdef can accept and return Python objects. I suppose it is just where they can be called from that makes a difference

jnothman · 2016-04-14T23:33:48Z

They can accept and return Python objects, but if something is typed as a
non-Python object, it will only perform automatic conversions for def or
cpdef.

cpdef is not the solution here. But I also think we can't get cdef to work.

On 15 April 2016 at 06:51, Manoj Kumar notifications@github.com wrote:

The difference between def and cdef lies in whether the function needs to
be exposed to Python meaning it needs to accept Python objects as input.

Is that true? From here (
http://docs.cython.org/src/reference/language_basics.html#functions-and-methods)
it says both def and cdef can accept and return Python objects. I suppose
it is just where they can be called from that makes a difference

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6659 (comment)

yenchenlin · 2016-04-15T00:06:35Z

Hello @rvraghav93 , thanks!
However, motivated by @jnothman , I tried a little script to test it.

Any idea why following code works?
It also call a cdef function inside a python function

def function(int i):
    _function(i)

cdef void _function(int i):
    print i

function(10)

jnothman · 2016-04-15T00:14:02Z

The reason why the current code does not work is not because it is a cdef called from a def. It is because the Cython translator needs to be able to resolve which type among a fused type to select for a particular input argument. But the translator doesn't know anything about X.data and doesn't know anything about numpy ndarrays. So it can't do that static typing without full conversion from arbitrary Python object, hence def/cpdef.

yenchenlin · 2016-04-15T02:20:12Z

Yeah @jnothman , you are right.
cpdef is not the solution here, I've tried it and it still break.

Btw, "private" isn't really the correct description. The difference between def and cdef lies in whether the function needs to be exposed to Python meaning it needs to accept Python objects as input (and hence handle any argument list parsing and conversion) and return a Python object as output.

If so, why can't we leave it as def here?

MechCoder · 2016-04-15T03:39:19Z

Because of the same reason as @jnothman had mentioned. cdef is faster because of static typing, see (http://notes-on-cython.readthedocs.org/en/latest/classes.html) for a quick example.

Also "private" because you cannot import _inplace_csr_row_normalize_l1 outside ~~sparsefuncs_fast.pyx~~ sklearn. (Inside sklearn you can do a cimport)

But it seems like fused memoryviews can be an option though. Shouldn't be too tough to try it out. (http://docs.cython.org/src/userguide/fusedtypes.html#fused-types-and-arrays)

yenchenlin · 2016-04-15T04:07:00Z

@MechCoder Can you give an brief example for how to use fused memoryviews to solve current problem?

(sorry if this is obvious)

jnothman · 2016-04-15T04:13:17Z

Yes, we can, and should, leave it as def here, IMO.

On 15 April 2016 at 14:07, Yen notifications@github.com wrote:

@MechCoder https://github.com/MechCoder Can you give an brief example
for how to use fused memoryviews to solve current problem?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6659 (comment)

MechCoder · 2016-04-15T04:18:00Z

@yenchenlin1994 Similar to what is being done over here (https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/linear_model/cd_fast.pyx#L287)

But I agree with @jnothman , this is unlikely to be a bottleneck and not worth the effort.

jnothman · 2016-04-15T04:29:57Z

@MechCoder, that example involves neither cdef, nor fused types. Here the issue is the combination of cdef, fused types and numpy arrays.

MechCoder · 2016-04-15T04:46:40Z

I wrongly extrapolated the logic in spite of your comments. (That sums up a really bad day at work)

yenchenlin · 2016-04-15T06:26:08Z

Hey guys, I think that we agree we should still use def here since cdef function can't figure out which type among a fused type to select for a particular input argument?
If so, I will close this PR.

However, there should be a PR that make cython compiler directives global and also move the comments here from subfunction to function.
Should I create a new one?

ping @jnothman @MechCoder

MechCoder · 2016-04-15T07:11:09Z

Please go ahead

raghavrv reviewed Apr 14, 2016
View reviewed changes

yenchenlin force-pushed the make-sparse-utils-funtions-private branch 2 times, most recently from 714f34f to 82b66e4 Compare April 14, 2016 12:14

jnothman changed the title ~~Make subfunctions of sparsefuncs private~~ [MRG+1] Make subfunctions of sparsefuncs private Apr 14, 2016

yenchenlin changed the title ~~[MRG+1] Make subfunctions of sparsefuncs private~~ [WIP] Make subfunctions of sparsefuncs private Apr 14, 2016

jnothman reviewed Apr 14, 2016
View reviewed changes

yenchenlin changed the title ~~[WIP] Make subfunctions of sparsefuncs private~~ [MRG+1] Make subfunctions of sparsefuncs private Apr 14, 2016

jnothman reviewed Apr 14, 2016
View reviewed changes

jnothman changed the title ~~[MRG+1] Make subfunctions of sparsefuncs private~~ [MRG] Make subfunctions of sparsefuncs private Apr 14, 2016

yenchenlin force-pushed the make-sparse-utils-funtions-private branch from 82b66e4 to 975b600 Compare April 14, 2016 15:59

yenchenlin force-pushed the make-sparse-utils-funtions-private branch from 975b600 to 5111288 Compare April 15, 2016 06:15

make subfunction private

2f854c3

yenchenlin force-pushed the make-sparse-utils-funtions-private branch from 5111288 to 2f854c3 Compare April 15, 2016 06:16

yenchenlin changed the title ~~[MRG] Make subfunctions of sparsefuncs private~~ [MRG] Make cython compiler directives global Apr 15, 2016

yenchenlin changed the title ~~[MRG] Make cython compiler directives global~~ [MRG] Make subfunctions of sparsefuncs private Apr 15, 2016

yenchenlin closed this Apr 15, 2016

This was referenced Apr 16, 2016

[MRG+2] Use fused types in sparse mean variance functions #6593

Merged

[MRG+1] Allows KMeans/MiniBatchKMeans to use float32 internally by using cython fused types #6430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Make subfunctions of sparsefuncs private #6659

[MRG] Make subfunctions of sparsefuncs private #6659

yenchenlin commented Apr 14, 2016

raghavrv Apr 14, 2016

raghavrv Apr 14, 2016

jnothman Apr 14, 2016

jnothman Apr 14, 2016

yenchenlin Apr 14, 2016

raghavrv Apr 14, 2016

jnothman Apr 14, 2016

MechCoder Apr 14, 2016

jnothman commented Apr 14, 2016

yenchenlin commented Apr 14, 2016

raghavrv commented Apr 14, 2016

jnothman Apr 14, 2016

jnothman commented Apr 14, 2016

jnothman commented Apr 14, 2016

jnothman Apr 14, 2016

jnothman Apr 14, 2016

yenchenlin Apr 14, 2016

MechCoder Apr 14, 2016

yenchenlin commented Apr 14, 2016

raghavrv commented Apr 14, 2016 •

edited

Loading

MechCoder commented Apr 14, 2016 •

edited

Loading

jnothman commented Apr 14, 2016

yenchenlin commented Apr 15, 2016

jnothman commented Apr 15, 2016 •

edited

Loading

yenchenlin commented Apr 15, 2016 •

edited

Loading

MechCoder commented Apr 15, 2016 •

edited

Loading

yenchenlin commented Apr 15, 2016 •

edited

Loading

jnothman commented Apr 15, 2016

MechCoder commented Apr 15, 2016

jnothman commented Apr 15, 2016

MechCoder commented Apr 15, 2016

yenchenlin commented Apr 15, 2016 •

edited

Loading

MechCoder commented Apr 15, 2016

		@@ -275,13 +275,16 @@ def incr_mean_variance_axis0(X, last_mean, last_var, unsigned long last_n):
		@cython.wraparound(False)

[MRG] Make subfunctions of sparsefuncs private #6659

[MRG] Make subfunctions of sparsefuncs private #6659

Conversation

yenchenlin commented Apr 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Apr 14, 2016

yenchenlin commented Apr 14, 2016

raghavrv commented Apr 14, 2016

Choose a reason for hiding this comment

jnothman commented Apr 14, 2016

jnothman commented Apr 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yenchenlin commented Apr 14, 2016

raghavrv commented Apr 14, 2016 • edited Loading

MechCoder commented Apr 14, 2016 • edited Loading

jnothman commented Apr 14, 2016

yenchenlin commented Apr 15, 2016

jnothman commented Apr 15, 2016 • edited Loading

yenchenlin commented Apr 15, 2016 • edited Loading

MechCoder commented Apr 15, 2016 • edited Loading

yenchenlin commented Apr 15, 2016 • edited Loading

jnothman commented Apr 15, 2016

MechCoder commented Apr 15, 2016

jnothman commented Apr 15, 2016

MechCoder commented Apr 15, 2016

yenchenlin commented Apr 15, 2016 • edited Loading

MechCoder commented Apr 15, 2016

raghavrv commented Apr 14, 2016 •

edited

Loading

MechCoder commented Apr 14, 2016 •

edited

Loading

jnothman commented Apr 15, 2016 •

edited

Loading

yenchenlin commented Apr 15, 2016 •

edited

Loading

MechCoder commented Apr 15, 2016 •

edited

Loading

yenchenlin commented Apr 15, 2016 •

edited

Loading

yenchenlin commented Apr 15, 2016 •

edited

Loading