Repository: scikit-learn/scikit-learn
Pull request: clarify that sklearn.metric.pairwise doesn't provide callables for pairwise_kernel
Fixes #10890. Closes #13111.

Changing the current behavior in the suggested way just means that computations are unnecessarily slower because the user didn't read the docstring carefully. 

Problem statement:
`_pairwise_callable` function doesn't work with pairwise metrics functions from sklearn itself
#### Description

`metrics.pairwise` functions (`rbf_kernel` for instance) throw error due to unexpected 1-D array instead of 2-D array when called from _pairwise_callable.

I discovered the bug when I have been using `sklearn.kernel_approximation.Nystroem` with `sklearn.metrics.pairwise.rbf_kernel`. When Nystroem wants to compute the pairwise metric between the examples it asks for `_pairwise_callable` to do it and inside this function there is this thing:

```python
out = np.zeros((X.shape[0], Y.shape[0]), dtype='float')
        iterator = itertools.combinations(range(X.shape[0]), 2)
        for i, j in iterator:
            out[i, j] = metric(X[i], Y[j], **kwds)
```

if `metric` is a metric from `sklearn.metrics.pairwise` (I mean, the function, not the string) then it crashes because `X[i] `and `Y[j]` are both of shape `(d,)` instead of `(1, d)` and those metrics check if the input is 2D array.

So there is a problem. But it may be multiple fold:

  - I think my use case should work and for that, we only need to change the above code to the following:

```python
out = np.zeros((X.shape[0], Y.shape[0]), dtype='float')
        iterator = itertools.combinations(range(X.shape[0]), 2)
        for i, j in iterator:
            x_i = np.reshape(X[i], (1, -1))
            y_j = np.reshape(Y[j], (1, -1))
            out[i, j] = metric(x_i, y_j, **kwds)
```
  - One could say that to address my problem, I should use the string definition of kernel instead of the function directly. The problem is that the documentation of the `sklearn.metrics.pariwise.pairwise_kernels` function is contradictory because it says that valid values for metric are `['rbf', 'sigmoid', 'polynomial', 'poly', 'linear', 'cosine']` AND  "If metric is a string, it must be one of the metrics in `pairwise.PAIRWISE_KERNEL_FUNCTIONS`" which are defined at `sklearn.metrics.pairwise.kernel_metrics` and contains other metrics than above ('chi2' for instance). Anyway, if I must not use `_pairwise_callable` with pairwise metrics from within `sklearn` itself, then It should be said in the documentation.

What do you think?

#### Steps/Code to Reproduce

Example:
```python
import numpy as np
from sklearn.metrics.pairwise import rbf_kernel

subsample = np.random.rand(8, 10)
nys = Nystroem(kernel=rbf_kernel, gamma=1., n_components=subsample.shape[0])
nys.fit(subsample)
```

#### Versions
```
Linux-4.13.0-37-generic-x86_64-with-debian-stretch-sid
Python 3.5.4 |Anaconda, Inc.| (default, Nov 20 2017, 18:44:38) 
[GCC 7.2.0]
NumPy 1.14.2
SciPy 1.0.0
Scikit-Learn 0.19.1
```

Hints from issue discussion:
There are a few things in this issue. Here is what I could spot (I may have missed a few things because there is a lot of text in-between ...):
* the doc is inaccurate. A PR fixing that would be more than welcome!
* you expect to pass `kernel=rbf_kernel` and it does not work. Without having looked at it in detail, you may have a point. As you said this does not really affect usability of `Nystroem` because you can use `kernel='rbf'` instead. Have you tried to implement the change you are proposing, run the tests and see if something breaks?

Thank you for your answer. Sorry for not have been clearer. You have not missed anything.

I am not used to contributing on open-source projects, I wanted to share my point of view before doing anything. I put this on my todo list. You are right for `Nystroem`  this is how I achieved my goal.
hhmmm... we might have formerly allowed pairwise_distances to take 1d
arrays, and perhaps this is sensible... Removing that functionally would
have opened this problem if this case was not properly tested. Maybe we
should consider reinstating 1d support in pairwise metrics and testing this
properly.

``kernel='rbf'`` definitely works but I feel like what you want should also work and we should fix it.
I've run into this issue as well. I believe the main issue is with `_pairwise_callable` trying to be clever and only build half the kernel matrix (and then replicating it for the full matrix), by calling the metric function in a loop over rows of X and Y. The already implemented kernels, like `rbf_kernel` expect X and Y as matrices and are optimized to produce the full kernel matrix faster than any looping. Thus I believe all optimizations should be in the kernel functions themselves and removed from _pairwise_callable. And all user created kernel functions passed in would then have to function like the built in kernels, ie expecting matrices. I think this is the cleanest solution, but not sure what it might break...
I've run into the same issue as well. A simple solution I can think of that won't break anything is to check if the metric is in `PAIRWISE_KERNEL_FUNCTIONS.values()` in addition to `PAIRWISE_KERNEL_FUNCTIONS.keys()`.
I would argue that the actual problem is that we don't have an API that allows a user to pass a matrix-matrix distance function.
If we allowed the 1d case with the reshape as suggested by @lucgiffon that would make things much slower if passing the function instead of passing the string.
If we did the fix of @HanGuo97 that would mean some callables are treated different from other callables, which seems weird.

Unfortunately there is no way for us to know whether the callable the user passes allows matrix-matrix distances or only vector-vector distances.

@lucgiffon said
> "If metric is a string, it must be one of the metrics in pairwise.PAIRWISE_KERNEL_FUNCTIONS" which are defined at sklearn.metrics.pairwise.kernel_metrics and contains other metrics than above ('chi2' for instance). Anyway, if I must not use _pairwise_callable with pairwise metrics from within sklearn itself, then It should be said in the documentation.

I think the docs already say that it needs to be a string, but we can be more explicit in saying that the callables from sklearn are not supported, because supporting them *and* supporting the current interface would lead to computations that are much slower (unless we come up with a way to specify if a metric is vector-vector or matrix-matrix).

Another possibility would to go to matrix-matrix callables everywhere in scikit-learn, but that sounds like a rough deprecation...

Commit message: backticks

Patch to review:
diff --git a/sklearn/metrics/pairwise.py b/sklearn/metrics/pairwise.py
index b25c428da9e43..bb499192fd373 100644
--- a/sklearn/metrics/pairwise.py
+++ b/sklearn/metrics/pairwise.py
@@ -1678,8 +1678,11 @@ def pairwise_kernels(X, Y=None, metric="linear", filter_params=False,
         If metric is "precomputed", X is assumed to be a kernel matrix.
         Alternatively, if metric is a callable function, it is called on each
         pair of instances (rows) and the resulting value recorded. The callable
-        should take two arrays from X as input and return a value indicating
-        the distance between them.
+        should take two rows from X as input and return a value indicating
+        the distance between them. This means that callables from
+        ``sklearn.metrics.pairwise`` are not allowed, as they operate on
+        matrices, not single samples. Use the string identifying this metric
+        instead.
 
     filter_params : boolean
         Whether to filter invalid parameters or not.


Code review comments:
diff_hunk: @@ -1678,8 +1678,11 @@ def pairwise_kernels(X, Y=None, metric="linear", filter_params=False,
         If metric is "precomputed", X is assumed to be a kernel matrix.
         Alternatively, if metric is a callable function, it is called on each
         pair of instances (rows) and the resulting value recorded. The callable
-        should take two arrays from X as input and return a value indicating
-        the distance between them.
+        should take two rows from X as input and return a value indicating
+        the distance between them. This means that callables from
path: sklearn/metrics/pairwise.py
text: @user1:
Distance between them isn't right

@author:
should be kernel?

@author:
Fittingly I was just listening to this: https://www.youtube.com/watch?v=Qv5fqunQ_4I

diff_hunk: @@ -1678,8 +1678,11 @@ def pairwise_kernels(X, Y=None, metric="linear", filter_params=False,
         If metric is "precomputed", X is assumed to be a kernel matrix.
         Alternatively, if metric is a callable function, it is called on each
         pair of instances (rows) and the resulting value recorded. The callable
-        should take two arrays from X as input and return a value indicating
-        the distance between them.
+        should take two rows from X as input and return a value indicating
+        the distance between them. This means that callables from
+        ``sklearn.metrics.pairwise`` are not allowed, as they operate on
+        matrices, not single samples. Use the string identifying this metric
path: sklearn/metrics/pairwise.py
text: @user1:
```suggestion
        matrices, not single samples. Use the string identifying the kernel
```