@ogrisel @jnothman ?

It has something to do with the Travis cache related to #6043 , because I have removed sparsefuncs_fast.pxd but it shows that it still exists.

@arthurmensch Would you be able to have a look?

because I have removed sparsefuncs_fast.pxd but it shows that it still exists.

Something similar happened to @tguillemot recently in one of his PR. And it was a non pyc file that he added at the beginning of the PR and later removed it. The travis cache was not updated with that removal until he raised a new PR.

Oh that's not the case here, I just removed an existing pxd file. Must be something silly.

_centers_sparse doesn't have checks disabled. Please have a look through and see if it is safe to do so (i.e. is there code in that function with an index that could be out of range, e.g. user-supplied); otherwise, you may still benefit from a helper function.

@jnothman good point! I added the checks cython flags disabling the checks and they pass locally.

But you should be checking that when the checks are disabled, there's no possibility to segfault even if the input is pathological, which we don't often test in scikit-learn.

Sorry for being dense and I'm afraid my brain is not functioning to its full capacity but I don't completely understand your concern.

Before this pull request:

1. Checks were enabled in _centers_sparse
2. Checks were disabled in add_row_csr

After this pull request (and ignoring the last commit where I disabled the checks)

1. Checks are enabled in _centers_sparse (which now has add_row_csr)

If you afraid that by disabling checks in _centers_sparse, it is capable of segfaulting on wrong input, I can understand that. If we want to disable checks to make it faster we should do it separately
(thus reverting my last commit)

Or is your concern something else?

Reading your comment again, I have moved that separately #6677

Bounds checks and wraparound checks can be removed ONLY IF the code will
otherwise never try to take advantage of those Python indexing features
(i.e. x[i] for i > len(x) triggers IndexError; x[i] for -len(x) <= i < 0
returns x[len(i) - i]). So you have to look through when indexing is
performed in the code, assess what the indices are (where they get their
values) and if for expression x[i], i can ever be <0 or >=len(x) for any
data that can be passed to the function in question.

On 19 April 2016 at 16:56, Manoj Kumar notifications@github.com wrote:

Reading your comment again, I have moved that separately #6677
#6677

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

Oh, sorry for not being clear, my question wasn't about WHY the checks can or cannot be removed which I'm perfectly aware of, but WHY that had to be dealt in this PR

In other words, I hadn't disabled the Cython checks before you asked me to.

To clarify, my concern was that you lose the performance benefits of add_row_csr by merging it into a function with checks enabled.

Right, sorry for being short-sighted.

Ha! If we all apologised for such things we'd make change even slower... :)

Hello @jnothman and @MechCoder

What is the status of this PR currently?
Since cython checks in _centers_sparse has been disabled in #6677 ,
it seems there is no need to worry about performance degradation anymore.

This PR looks good to me.

+1

It's just that Travis is failing.

Uhh... whoops :)

Is it just me, or is Travis somehow using a previous version of the pxd which is deleted here...

Yes, there is some weird cache going on, that I don't really have the time to debug in the next few days.

@lesteve, would you be able to help out with what seems to be a Travis build cache issue? It seems to be retaining a deleted .pyx.

Maybe you can try clearing the cache, following this.

If everything else fails, why not create a new PR? I pushed this branch into my remote which has Travis set-up and everything seemed to work fine.

Hello @MechCoder , do you want to create a new PR for this?

@yenchenlin1994, you can assume that any PR which is in your GSoC scope and not clearly being worked on by someone else is good for you to take over. Make that PR yourself.

Hello @lesteve , I've opened another PR but it seems that it is still failing.

Ah I got it, thanks @jnothman.
Then can you give me some opinions on #6785? 😄

After completing this PR and #6785, sparsefuncs would support Cython fused type thoroughly.
And then we can continue to work on #6430 .

Thanks @lesteve, clearing the caches seems to have worked.

Thanks for sorting it out. Can anyone give a second pass?

cc: @amueller @ogrisel @TomDLT ?

lgtm

@MechCoder merge?

thx