Based on the cython doc, floating can only be float or double, therefore the else: raise ValueError blocks can never be reached.

Please replace those if floating is float / elif floating is double / else constructs by if floating is float / else constructs instead.

Please also feel free to squash those commits.

Other than that, LGTM, +1 for merge with a new entry in doc/whats_new.rst.

Thanks for your review and the comments.
I squashed the old commits and addressed your comments.

Please squash the new commit as well. Commits with a message such as "Adresses several comments" are not interesting when reviewing the history of a file 6 months from now :)

Ok, thanks, I squashed them. :)

Hello @ssaeger ,
would you please elaborate on

Unfortunately I was not able to add support to handle sparse float32 input data as float32 internally, so that this is still converted to float64. This would require many changes in sparsefuncs_fast.pyx.

What do you mean by internally?
Thanks

Hi @yenchenlin1994 ,
at the moment sparsefuncs_fast.pyx only accepts np.float64 and I did not want to make changes to that in this PR since it appeared to me that this would be a change that affects other parts too and is not only related to this PR so that a separate PR would be simpler to review.

With internally I mean that if the user inputs sparse data with dtype np.float32 it will be converted to np.float64 in order to use the functionality of sparsefuncs_fast.pyx. So it is not visible to the user and happens internally without the user noticing it.

I hope this makes it a bit more clear. :)

@ssaeger great thanks for your explanation.
Super clear :) 👍

It would be great to wait for #6593

Seems like we still need add_row_csr to support both dtypes.

Yeah ... I'll do that.

Hello @MechCoder ,
It seems like add_row_csr is a cdef function right now.
And base on the discussion here, we can only make a function support fused types by turning it into a def function.

Am I on the right track?
Or we need to do a benchmark to evaluate this memory-speed trade-off?

It seems like add_row_csr is a cdef function right now. And base on the discussion here, we can only make a function support fused types by turning it into a def function.

It's actually possible that we could have used a cdef there (though the benefit is not as great), but I'm fairly sure we can here if we used typed memoryviews instead of np.ndarrays. For example, this compiles:

from cython cimport floating

cdef floating a1(floating[:] x):
    return x[0]

def b(X, floating y):
    cdef floating[:] X_data = X.data
    return a1(X_data)

(Also, should we be doing the fused typing of CSR/CSC sparse matrix indices and indptr at the same time as we make these changes? Should we be adding nogil where appropriate?)

Hello @jnothman , I see the code you paste compiles.

though the benefit is not as great

But I wonder why the benefit there is not as great as here?
Isn't cdef faster than def?

Because you're going from Cython to Cython here not from Python to Cython as there; and because here it is called repeatedly, while calculating means is not so often repeated.

Because you're going from Cython to Cython here not from Python to Cython as there; and because here it is called repeatedly, while calculating means is not so often repeated

Thanks a lot! Now I understand 😄

So, do you mean I can simply replace np.ndarray[np.float64_t, ndim=1] data with floating[:] data in add_row_csr?
However, that didn't compile, I think I probably miss something.

(I just learned memoryviews, sorry if this is obvious)

I actually removed the function here (#6676)

Note about memory views: they tend to not work correctly with readonly memory buffers:

https://mail.python.org/pipermail/cython-devel/2013-February/003384.html

See the discussion in pandas-dev/pandas#10043 for an example on how this can cause problems when working with readonly mapped data.

If we can I think we should try to stick to the ndarray cython dtype whenever this is a user-provided numpy array to make it possible to use memory mapped data (and thus joblib parallel).

@yenchenlin1994 Since Olivier just said that using memoryviews would break joblib parallelism. You have two options.

1. Make the cdef in sparsefuncs_fast.pyx a def. Intuitively this should cause speed regressions. You should benchmark thoroughly to see the differences in speed if any.
2. Continue my work at [MRG+1] Disable cython checks in _centers_sparse #6677 and [MRG+2] MAINT: Remove add_row_csr #6676 to remove the function all together. In that case Joel's comments on the PR's would be more than helpful.

@MechCoder thanks for your help!
Will first try 1 and adopt 2 if speed decline alot.
What do you think?

The first option seems to be the easier to try out.

@ssaeger It seems we have fused types support for all sparsefuncs. Would you be able to rebase or would you prefer @yenchenlin1994 to do the rebase and testing for the sparse case as part of GSoC?

@MechCoder At the moment I'm very busy. Therefore I would prefer @yenchenlin1994 to continue the work on this.

Sure!

@ssaeger thanks for your hard work.

Hello @ssaeger , really sorry to interrupt you.
Can you provide the memory profiling script which generates the figures above?

hey, no problem 😃

import numpy as np
from sklearn.cluster import KMeans

@profile
def fit_est():
    estimator.fit(X)

np.random.seed(5)
X = np.random.rand(500000, 20)
X = np.float32(X)

estimator = KMeans()
fit_est()

I used following memory profiler: https://pypi.python.org/pypi/memory_profiler
Just install it and use it with:
mprof run <executable>
mprof plot

memory_profiler is covered at http://scikit-learn.org/dev/developers/performance.html. Perhaps you should familiarise yourself with the rest of the tips there (including some for Cython, but perhaps outdated).

Thanks @ssaeger and @jnothman for the inputs.
Will go through those tips!

Hello guys, I've created #6846 to replace this PR, maybe we can close this one?