Informative exception when passing a sparse matrix

[urigoren]

Proposed solution to
#7565

[perimosocordiae]

The docs indicate that NotImplementedError should be used for abstract methods or in-progress methods. I don't think there's a plan to support sparse inputs here, so TypeError might be more appropriate.

Also, I'd like a shorter message. Maybe something like: "pdist doesn't support sparse matrix inputs, use sklearn.metrics.pairwise_distances instead."

[urigoren]

Thank you, Done...

[perimosocordiae]

typo: skearn -> sklearn

Also wrap the string to fit in 80 columns, please.

[urigoren]

Thanks, Done !

[perimosocordiae]

scipy.sparse needs to be imported before use.

[urigoren]

@perimosocordiae , please re-review

[perimosocordiae]

Looks good, thanks @urigoren!

My final concern is that this will cause scipy.spatial to depend on importing scipy.sparse, which will slightly increase the time it takes to run import scipy.spatial the first time. I don't expect this to be a big slowdown, though, so I'm +1 to merge.

[rgommers]

This doesn't look quite right to me:

• _copy_array_if_base_present is called from other functions than pdist (e.g. squareform), so is not the right place to put this check
• adding coupling between scipy submodules just for an exception message is slightly questionable. there are many types of inputs that aren't accepted.......
  EDIT: and there lots of other scipy functions where one could add this check - doesn't seem desirable

[rgommers]

Looks like a documentation change is a better way to go about this. And perhaps it would make sense to move the scikit-learn distance metrics into scipy.spatial?

[urigoren]

@rgommers , So what do you suggest to do next? Move this check to _copy_array_if_base_present or copy the code from sklearn's code ?

[perimosocordiae]

For now, let's just add a warning not to use sparse matrix inputs to the docstring of pdist and cdist, perhaps in the "Notes" section.

I do think that eventually scipy.spatial should grow sparse matrix support similar to what scikit-learn provides, but that will be a large effort that needs to be designed carefully.

[urigoren]

What are the downsides of merging this PR, only the possible increase in loading time ?
Correct me if I'm wrong, but that can be tested with a simple %timeit call, right ?

I agree with @perimosocordiae , that we definitely want pdist and cdist to support sparse matrices in the future,
but until that happens, what's the harm of having an informative exception ?

[rgommers]

What are the downsides of merging this PR, only the possible increase in loading time ?
Correct me if I'm wrong, but that can be tested with a simple %timeit call, right ?

No you can't. Measuring import time is quite tricky to get right (after the first import it's a lookup in globals so much faster) - we had some discussion on that on the numpy-discussion list last year.

Either way, import time is not the main issue. The issues are the two I listed in #7566 (comment).
EDIT: for the first one, just try squareform(some_sparse_matrix) to see what is wrong

[rgommers]

I do think that eventually scipy.spatial should grow sparse matrix support similar to what scikit-learn provides, but that will be a large effort that needs to be designed carefully.

Agreed

For now, let's just add a warning not to use sparse matrix inputs to the docstring of pdist and cdist, perhaps in the "Notes" section.

+1

[ev-br]

Just noting that there's a similar utility function instead of a direct use of issparse, used e.g. in scipy.linalg IIRC: https://github.com/scipy/scipy/blob/master/scipy/_lib/_util.py#L192

[urigoren]

@rgommers , @ev-br , I fixed the sparsity check as you suggested.

The test seem to pass (despite what the CI says), it's an accuracy issue on python 3.5 - please have a look at the logs.

Could you please merge ?