alexbovet / flow_stability

Flow stability framework for dynamic community detection in temporal networks
GNU Lesser General Public License v3.0
24 stars 4 forks source link

ditch `sparse_dot_mkl` in favor of `scipy` compiled with `mkl`? #46

Closed j-i-l closed 3 weeks ago

j-i-l commented 2 months ago

... One thing that bugs me with the current approach of using MKL is that MKL also provides BLAS and LAPACK and thus could be linked against when building scipy. In doing so we should be able to get a scipy version that relies on MKL for matrix operations and we would not need to write any extra code and checks if some package is installed or not.

An approach might be to remove the sparse_dot_mkl package and add instructions (or simply links) to how one can build scipy with MKL for BLAS and LAPAK. For now (basically as long as dependency groups are not really a thing) it would be rather cumbersome to provide a "[mkl]" installation option but we could even add a simple 'how-to' section if using the intel based scipy from the anaconda mirror, or the inter-scipy python package provide functional, pre-compiled versions of scipy with mkl.

_Originally posted by @j-i-l in https://github.com/alexbovet/flow_stability/issues/35#issuecomment-2327446958_

j-i-l commented 2 months ago

@alexbovet what is your opinion on this?

alexbovet commented 2 months ago

AFAIK sparse_dot_mkl can do multithreaded sparse matrix multiplication which scipy cannot do at the moment, even if it is linked with MKL . I don't think it's a problem to have both and let user have scipy linked with whatever BLAS they want? But we should keep sparse_dot_mkl as optional because, for example, I'm not sure you can use it on a Mac without an intel processor. Note that there is a pull request to add parallelism to scipy csr_matrices. (But sparse_dot_mkl also has the gram_matrix_mkl which we use).

j-i-l commented 3 weeks ago

I see. Let's just keep the sparse_dot_mkl as optional dependency then. We can return to this if the pull request makes it into scipy