Repository: scikit-learn/scikit-learn
Pull request: ENH Adds `metric_params` parameter to pass to metrics in `TSNE`
#### Reference Issues/PRs
Fixes #11793 
Resolves #21805 (stalled)

#### What does this implement/fix? Explain your changes.
PR #21805: Adds a new `metric_params` argument to the TSNE constructor. It defaults to `None`, and can be used to pass keyword args to the chosen metric.

This PR: Addresses remaining review comments and merges with main.
#### Any other comments?


Problem statement:
Error on  method_parameters  with TSNE from sklearn with mahalanobis metric

#### Description


With TSNE from sklearn v0.19.2 python 3.6  with mahalanobis metric I am getting following error
ValueError: Must provide either V or VI for Mahalanobis distance
How to provide an **method_parameters** for the Mahalanobis distance?

#### Steps/Code to Reproduce
from sklearn.manifold import TSNE      
tsne = TSNE( verbose=1, perplexity=40, n_iter=250,learning_rate=50, random_state=0,metric='mahalanobis')
pt=data.sample(frac=0.1).values
tsne_results = tsne.fit_transform(pt)
#### Expected Results
no error and metricless calculation:)

Hints from issue discussion:
Some workaround is suggested on SO:
https://stackoverflow.com/questions/51766963/tsne-from-sklearn-with-mahalanobis-metric/51767948#51767948
This error comes when TSNE `method` param is not `'exact'`. In this case, NearestNeighbors is used.
Even in that, this only arises when `algorithm` param is set as `'ball_tree'` or `'kd_tree'` in `NearestNeighbors` from this [code snippet](https://github.com/scikit-learn/scikit-learn/blob/f0ab589f1541b1ca4570177d93fd7979613497e3/sklearn/neighbors/base.py#L225).

In this case, the distance metric is initialized from [this code](https://github.com/scikit-learn/scikit-learn/blob/f0ab589f1541b1ca4570177d93fd7979613497e3/sklearn/neighbors/binary_tree.pxi#L1052):

    ...
    self.dist_metric = DistanceMetric.get_metric(metric, **kwargs)
    ...

In case of Mahalanobis, `DistanceMetric.pairwise()` requires a V or VI param.

If the `algorithm` is set as `'brute'` in NearestNeigbors, or in tsne, the `method == 'exact'`, then the distances are calculated from 

    distances = pairwise_distances(X, metric=self.metric)

as specified in [code for NearestNeighbors here](https://github.com/scikit-learn/scikit-learn/blob/f0ab589f1541b1ca4570177d93fd7979613497e3/sklearn/neighbors/base.py#L359) and [code for TSNE here](https://github.com/scikit-learn/scikit-learn/blob/f0ab589f1541b1ca4570177d93fd7979613497e3/sklearn/manifold/t_sne.py#L690)

In both the type of cases the metric = "Mahalanobis", so I am not sure, why its treated differently in above cases from `DistanceMetric.pairwise()` and `pairwise_distances()`!


Code to duplicate the issue:

```

from sklearn.datasets import load_boston
X = load_boston().data

from sklearn.manifold import TSNE
tsne = TSNE(verbose=1, 
            perplexity=40, 
            n_iter=250,learning_rate=50, 
            random_state=0,
            metric='mahalanobis')
tsne_results = tsne.fit_transform(X)
```

If we replace `load_boston` with `load_iris` here (because then the `algorithm` will be set to `'brute'`), or explicitly put `method='exact'` (which will not use `NearestNeighbors`) in TSNE constructor, then the error doesn't occur.
easiest might be to use a precomputed distance matrix?

But the main issue here is why is same distance being treated differently? 

```
distances = pairwise_distances(X, metric='mahalanobis')

VS 

distances = DistanceMetric.get_metric(metric = 'mahalanobis').pairwise(X)
```
That seems to be due to a quirk in scipy.spatial.distance.cdist which seems to not require the required arguments of 'mahalanobis'. Perhaps this can be fixed upstream.

But it doesn't fix the fact that TSNE does not have a `metric_params` parameter; it probably should.
Hey jnothman, is the way forward with this issue to include a metric_params parameter? If yes, I'll go for it. 
I think so

Can I work on this, provided @udaykeith is not working on this? 
Go ahead Muneeb, this has been on the backburner for far too long.

On Mon, Oct 8, 2018 at 12:45 PM Muneeb Aadil <notifications@github.com>
wrote:

> Can I work on this, provided @udaykeith <https://github.com/udaykeith> is
> not working on this?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/11793#issuecomment-427739433>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AZTpWzWoNweH0-I-xahQk2Csn2Ek5DYxks5uivuagaJpZM4V31vJ>
> .
>

Hi there,

I'm also new in open software and would like very much to contribute to scikit-learn. Is this issue still without anyone working on it? I'll wait for a day or two and if no responses I'll open a pull request.

Commit message: Adds PR number to changelog

Patch to review:
diff --git a/doc/whats_new/v1.1.rst b/doc/whats_new/v1.1.rst
index ee0f75b92c789..e83afb03c3702 100644
--- a/doc/whats_new/v1.1.rst
+++ b/doc/whats_new/v1.1.rst
@@ -566,6 +566,11 @@ Changelog
   in eigen_solvers `lobpcg` and `amg` to improve their numerical stability.
   :pr:`21565` by :user:`Andrew Knyazev <lobpcg>`.
 
+- |Enhancement| added `metric_params` to :class:`manifold.TSNE` constructor for
+  additional parameters of distance metric to use in optimization.
+  :pr:`21805` by :user:`Jeanne Dionisi <jeannedionisi>` and :pr:`22685` by
+  :user:<Meekail Zain `micky774`>.
+
 :mod:`sklearn.metrics`
 ......................
 
diff --git a/sklearn/manifold/_t_sne.py b/sklearn/manifold/_t_sne.py
index 2d5c8f9fd6d45..809f510b9f8b8 100644
--- a/sklearn/manifold/_t_sne.py
+++ b/sklearn/manifold/_t_sne.py
@@ -606,6 +606,11 @@ class TSNE(BaseEstimator):
         the distance between them. The default is "euclidean" which is
         interpreted as squared euclidean distance.
 
+    metric_params : dict, default = None
+        Additional keyword arguments for the metric function.
+
+        .. versionadded:: 1.1
+
     init : {'random', 'pca'} or ndarray of shape (n_samples, n_components), \
             default='random'
         Initialization of embedding. Possible options are 'random', 'pca',
@@ -744,6 +749,7 @@ def __init__(
         n_iter_without_progress=300,
         min_grad_norm=1e-7,
         metric="euclidean",
+        metric_params=None,
         init="warn",
         verbose=0,
         random_state=None,
@@ -760,6 +766,7 @@ def __init__(
         self.n_iter_without_progress = n_iter_without_progress
         self.min_grad_norm = min_grad_norm
         self.metric = metric
+        self.metric_params = metric_params
         self.init = init
         self.verbose = verbose
         self.random_state = random_state
@@ -885,8 +892,9 @@ def _fit(self, X, skip_num_points=0):
                     # Also, Euclidean is slower for n_jobs>1, so don't set here
                     distances = pairwise_distances(X, metric=self.metric, squared=True)
                 else:
+                    metric_params_ = self.metric_params or {}
                     distances = pairwise_distances(
-                        X, metric=self.metric, n_jobs=self.n_jobs
+                        X, metric=self.metric, n_jobs=self.n_jobs, **metric_params_
                     )
 
             if np.any(distances < 0):
@@ -921,6 +929,7 @@ def _fit(self, X, skip_num_points=0):
                 n_jobs=self.n_jobs,
                 n_neighbors=n_neighbors,
                 metric=self.metric,
+                metric_params=self.metric_params,
             )
             t0 = time()
             knn.fit(X)
diff --git a/sklearn/manifold/tests/test_t_sne.py b/sklearn/manifold/tests/test_t_sne.py
index c601d956e6f29..78b4b77ad42a0 100644
--- a/sklearn/manifold/tests/test_t_sne.py
+++ b/sklearn/manifold/tests/test_t_sne.py
@@ -1198,6 +1198,38 @@ def test_tsne_n_jobs(method):
     assert_allclose(X_tr_ref, X_tr)
 
 
+# TODO: Remove filterwarnings in 1.2
+@pytest.mark.filterwarnings("ignore:.*TSNE will change.*:FutureWarning")
+def test_tsne_with_mahalanobis_distance():
+    """Make sure that method_parameters works with mahalanobis distance."""
+    random_state = check_random_state(0)
+    n_features = 10
+    n_samples = 300
+    X = random_state.randn(n_samples, n_features)
+    default_params = {
+        "perplexity": 40,
+        "n_iter": 250,
+        "learning_rate": "auto",
+        "n_components": 3,
+        "random_state": 0,
+    }
+
+    # 1. raises error here (original issue)
+    tsne = TSNE(metric="mahalanobis", **default_params)
+    msg = "Must provide either V or VI for Mahalanobis distance"
+    with pytest.raises(ValueError, match=msg):
+        tsne.fit_transform(X)
+
+    # 2. check for correct answer
+    precomputed_X = squareform(pdist(X, metric="mahalanobis"), checks=True)
+    ref = TSNE(metric="precomputed", **default_params).fit_transform(precomputed_X)
+
+    now = TSNE(
+        metric="mahalanobis", metric_params={"V": np.cov(X.T)}, **default_params
+    ).fit_transform(X)
+    assert_allclose(ref, now)
+
+
 @pytest.mark.filterwarnings("ignore:The PCA initialization in TSNE will change")
 # FIXME: remove in 1.3 after deprecation of `square_distances`
 def test_tsne_deprecation_square_distances():


Code review comments:
diff_hunk: @@ -1198,6 +1198,38 @@ def test_tsne_n_jobs(method):
     assert_allclose(X_tr_ref, X_tr)
 
 
+# TODO: Remove filterwarnings in 1.2
+@pytest.mark.filterwarnings("ignore:.*TSNE will change.*:FutureWarning")
+def test_tsne_with_mahalanobis_distance():
+    """Make sure that method_parameters works with mahalanobis distance."""
+    random_state = check_random_state(0)
+    n_features = 10
+    n_samples = 300
path: sklearn/manifold/tests/test_t_sne.py
text: @user1:
```suggestion
    n_samples, n_features = 300, 10
```

diff_hunk: @@ -1198,6 +1198,38 @@ def test_tsne_n_jobs(method):
     assert_allclose(X_tr_ref, X_tr)
 
 
+# TODO: Remove filterwarnings in 1.2
+@pytest.mark.filterwarnings("ignore:.*TSNE will change.*:FutureWarning")
+def test_tsne_with_mahalanobis_distance():
+    """Make sure that method_parameters works with mahalanobis distance."""
+    random_state = check_random_state(0)
+    n_features = 10
+    n_samples = 300
+    X = random_state.randn(n_samples, n_features)
+    default_params = {
+        "perplexity": 40,
+        "n_iter": 250,
+        "learning_rate": "auto",
+        "n_components": 3,
+        "random_state": 0,
+    }
+
+    # 1. raises error here (original issue)
+    tsne = TSNE(metric="mahalanobis", **default_params)
+    msg = "Must provide either V or VI for Mahalanobis distance"
+    with pytest.raises(ValueError, match=msg):
+        tsne.fit_transform(X)
+
+    # 2. check for correct answer
+    precomputed_X = squareform(pdist(X, metric="mahalanobis"), checks=True)
+    ref = TSNE(metric="precomputed", **default_params).fit_transform(precomputed_X)
+
+    now = TSNE(
+        metric="mahalanobis", metric_params={"V": np.cov(X.T)}, **default_params
+    ).fit_transform(X)
+    assert_allclose(ref, now)
path: sklearn/manifold/tests/test_t_sne.py
text: @user1:
```suggestion
    assert_allclose(X_trans, X_trans_expected)
```

diff_hunk: @@ -1198,6 +1198,38 @@ def test_tsne_n_jobs(method):
     assert_allclose(X_tr_ref, X_tr)
 
 
+# TODO: Remove filterwarnings in 1.2
+@pytest.mark.filterwarnings("ignore:.*TSNE will change.*:FutureWarning")
+def test_tsne_with_mahalanobis_distance():
+    """Make sure that method_parameters works with mahalanobis distance."""
+    random_state = check_random_state(0)
+    n_features = 10
+    n_samples = 300
+    X = random_state.randn(n_samples, n_features)
+    default_params = {
+        "perplexity": 40,
+        "n_iter": 250,
+        "learning_rate": "auto",
+        "n_components": 3,
+        "random_state": 0,
+    }
+
+    # 1. raises error here (original issue)
+    tsne = TSNE(metric="mahalanobis", **default_params)
+    msg = "Must provide either V or VI for Mahalanobis distance"
+    with pytest.raises(ValueError, match=msg):
+        tsne.fit_transform(X)
+
+    # 2. check for correct answer
+    precomputed_X = squareform(pdist(X, metric="mahalanobis"), checks=True)
+    ref = TSNE(metric="precomputed", **default_params).fit_transform(precomputed_X)
path: sklearn/manifold/tests/test_t_sne.py
text: @user1:
`X_trans_expected`

diff_hunk: @@ -606,6 +606,11 @@ class TSNE(BaseEstimator):
         the distance between them. The default is "euclidean" which is
         interpreted as squared euclidean distance.
 
+    metric_params : dict, default = None
path: sklearn/manifold/_t_sne.py
text: @user1:
```suggestion
    metric_params : dict, default=None
```

diff_hunk: @@ -1198,6 +1198,38 @@ def test_tsne_n_jobs(method):
     assert_allclose(X_tr_ref, X_tr)
 
 
+# TODO: Remove filterwarnings in 1.2
+@pytest.mark.filterwarnings("ignore:.*TSNE will change.*:FutureWarning")
+def test_tsne_with_mahalanobis_distance():
+    """Make sure that method_parameters works with mahalanobis distance."""
+    random_state = check_random_state(0)
+    n_features = 10
+    n_samples = 300
+    X = random_state.randn(n_samples, n_features)
+    default_params = {
+        "perplexity": 40,
+        "n_iter": 250,
+        "learning_rate": "auto",
+        "n_components": 3,
+        "random_state": 0,
+    }
+
+    # 1. raises error here (original issue)
path: sklearn/manifold/tests/test_t_sne.py
text: @user1:
I don't think that we need this comment

diff_hunk: @@ -566,6 +566,11 @@ Changelog
   in eigen_solvers `lobpcg` and `amg` to improve their numerical stability.
   :pr:`21565` by :user:`Andrew Knyazev <lobpcg>`.
 
+- |Enhancement| added `metric_params` to :class:`manifold.TSNE` constructor for
+  additional parameters of distance metric to use in optimization.
+  :pr:`21805` by :user:`Jeanne Dionisi <jeannedionisi>` and :pr:`22685` by
+  :user:<Meekail Zain `micky774`>.
path: doc/whats_new/v1.1.rst
text: @user1:
```suggestion
  :user:`Meekail Zain <micky774>`.
```