[MRG] ENH Add parameter to sklearn.manifold.TSNE to transition to squaring non-Euclidean distance metrics by default (#12401) #17662

joshuacwnewton · 2020-06-23T01:54:47Z

Reference Issues/PRs

Fixes #12401
Supersedes and closes #12419

The plan for deprecating legacy squaring behaviour is summarized in this comment.

What does this implement/fix? Explain your changes.

Adds a square_distances parameter to TSNE. (default='legacy')
- True enables squaring for all distances metrics
- 'legacy' preserves existing behaviour, where distances will only be squared when metric='euclidean'. Raises a FutureWarning when a non-Euclidean metric is provided to alert the deprecation of legacy behavior. Setting True suppresses this warning.
Adds a parameter description to the docstring of TSNE
Adds tests to check square_distances and check that FutureWarning is raised properly.
Modifies existing tests to include square_distances=True to suppress FutureWarnings.

Any other comments?

~~I was uncertain about the usage of pytest.mark.parametrize in my tests. Some existing tests use it, while others iterate through lists of parameters. Style suggestions are appreciated.~~ Answered in feedback.
~~I was uncertain about the descriptions for my tests. Some use docstrings while others use a simple comment. Style suggestions are appreciated.~~ Answered in feedback.
I was uncertain about proposing a timeline for changing the default value and/or deprecating 'legacy' behaviour. Discussion in the issue (tSNE with non-Euclidean metric should be able to square the distances #12401) has become outdated, so I chose version numbers based on the deprecation section of the Contributing docs.

I presumed that adding square_distance could allow for negative precomputed distances in some cases (square_distance=True). However, `knn.fit()` does its own non-neg check, which forces precomputed distances to be positive regardless of the new square_distance parameter.

Tests cover the following areas: * Invalid square_distance parameter selection * TSNE functioning with metric=Euclidean and square_distance=legacy * TSNE functioning with square_distance=True/False

joshuacwnewton · 2020-06-23T15:23:52Z

It looks like adding a FutureWarning caused other TSNE tests to fail. I'm going to add @ignore_warnings(category=FutureWarning) to the relevant tests, as per the Contributing docs: "The warning should be caught in all other tests (using e.g., @pytest.mark.filterwarnings), and there should be no warning in the examples."

A related question: should I be running my local tests to fail on Warnings, then? I might have not been using the right pytest flags, as I didn't catch this before opening a PR. (EDIT: Found the relevant pytest docs. I needed to be adding -W error::FutureWarning to my local testing.)

joshuacwnewton · 2020-06-23T16:28:46Z

Pinging @flosincapite. I think this is ready to go from [WIP] -> [MRG]. :)

flosincapite

Looks great! There are some comments, but only because I was extremely picky :)

sklearn/manifold/_t_sne.py

sklearn/manifold/tests/test_t_sne.py

joshuacwnewton · 2020-06-23T17:37:06Z

Thanks a ton for taking the time to review, @flosincapite!

Euclidean handles squaring differently than other metrics. Euclidean is also slower for n_jobs > 1. So, the same call to pairwise_distances can't be used for every metric -- they have to be separated.

joshuacwnewton · 2020-06-24T17:25:24Z

sklearn/manifold/tests/test_t_sne.py

+@pytest.mark.parametrize('method', ['exact', 'barnes_hut'])
+@ignore_warnings(category=FutureWarning)
+def test_tsne_with_different_squaring_methods(method):
+    """Isolate failing behavior in
+    `test_tsne_with_legacy_euclidean_squaring.`"""
+    random_state = check_random_state(0)
+    n_components_original = 3
+    n_components_embedding = 2
+    X = random_state.randn(50, n_components_original).astype(np.float32)
+    X_precomputed_1 = pairwise_distances(X, metric='euclidean') ** 2
+    X_precomputed_2 = pairwise_distances(X, metric='euclidean', squared=True)
+    X_transformed_tsne_precomputed_1 = TSNE(
+        metric='precomputed', n_components=n_components_embedding,
+        random_state=0, method=method).fit_transform(X_precomputed_1)
+    X_transformed_tsne_precomputed_2 = TSNE(
+        metric='precomputed', n_components=n_components_embedding,
+        random_state=0, method=method).fit_transform(X_precomputed_2)
+    assert_array_equal(X_transformed_tsne_precomputed_1,
+                       X_transformed_tsne_precomputed_2)
+
+


I believe I've uncovered an existing issue with TSNE using this test.

This test isolates failing behaviour in another test, here. That test passes with method='exact', but fails with method='barnes_hut'.

For both tests, I believe the issue stems from TSNE's sensitivity to small changes in the input, which just so happens to be demonstrated nicely by two different squaring methods.

X_precomputed_1 = pairwise_distances(X, metric='euclidean') ** 2 X_precomputed_2 = pairwise_distances(X, metric='euclidean', squared=True)

Although mathematically the same, they might slightly differ in floating arithmetics. Then we can expect the results of t-sne to be slightly different too. Usually we use assert_allclose to compare arrays of floats. Since you're using float32, you can change the rtol to 1e-6 or even 1e-5.

Ah, sorry, it might have been better for me to have also included the output of the failing tests.

E Mismatched elements: 100 / 100 (100%) E Max absolute difference: 182.95767 E Max relative difference: 35.374535 E x: array([[-36.36463 , -60.091194], E [ 35.217983, -77.36719 ], E [-23.366423, -1.641921],... E y: array([[ -83.61592 , -40.051804], E [ -12.799014, 105.59048 ], E [ -2.169685, -25.965828],...

A small change in the distances (e.g. due to float arithmetic) seems to have caused a large difference in the TSNE output. So, I'm not sure assert_allclose would help here, but I could be mistaken.

Aha, that's good to know! I had assumed instability was a general issue, so thank you for clarifying. :)

A quick/dirty way to fix the test_tsne_with_legacy_euclidean_squaring test would be to replace this line with:

if method == 'barnes_hut': X_precomputed = pairwise_distances(X, metric='euclidean') ** 2 else: X_precomputed = pairwise_distances(X, metric='euclidean', squared=True)

That way the squaring method matches up with what's used internally in TSNE.

But, if I understand correctly, you're asking if there's a way configure TSNE to be stable enough that specifying the squaring method shouldn't be necessary?

Going out on a limb here, but the lack of stability could also have something do with with the use of randn to generate sample X values. Without much structure in our sample data, t-SNE might not find meaningful topology in the data, as is shown in this tutorial.

I wonder if it would be better to use a small toy dataset for t-SNE tests that has structure (e.g. a couple of small clusters in 3D). This could better reflect the intended use cases for t-SNE.

EDIT: I shouldn't just propose... I should try it out! I'll see if there's still an issue when using sklearn.make_blobs instead of randn. Turns out sklearn.make_blobs is already used in other t-SNE tests, even.

Yes, it does seem like make_blobs fixed the stability issues. :)

That makes a lot of sense. Good idea :)

Thank you for your help! Your questions pointed me in the right direction. :)

sklearn/manifold/tests/test_t_sne.py

t-SNE relies on structure in input data. When random data is used, t-SNE becomes very sensitive to slight changes in the input. So, make_blobs is used to create data with structure instead.

No longer necessary now that source of instability has been identified.

I thought I had written this test, but it wasn't mine! So, this change shouldn't have happened in the first place.

joshuacwnewton · 2020-07-01T16:04:42Z

Thanks much for the reviews @jeremiedbb. :)

sklearn/manifold/_t_sne.py

* Update warning to announce deprecation * Warn when 'legacy' is set. * Because 'legacy' now functions exactly the same as 'warn', remove 'warn' as a setting for 'square_distance'. * Update tests to reflect changes. * Update docstring to reflect changes.

jeremiedbb

LGTM, thanks @joshuacwnewton !

sklearn/manifold/_t_sne.py

Another enhancement was added to the sklearn.manifold section, causing a merge conflict.

NicolasHug

thanks @joshuacwnewton, made a first pass, mostly looks good but I'm not quite sure about the deprecation plan

doc/whats_new/v0.24.rst

sklearn/manifold/_t_sne.py

NicolasHug · 2020-07-10T19:17:12Z

sklearn/manifold/_t_sne.py

+                raise ValueError("All distances should be positive, the "
+                                 "metric given is not correct")
+
+            if self.metric != "euclidean" and self.square_distance is True:


Does this also work in interpreters that aren't CPython? I thought that True being a singleton was a CPython implementation detail

This might not be thorough enough, but a quick search yields:

IronPython: https://ironpython-test.readthedocs.io/en/latest/library/constants.html

Jython: https://www.jython.org/jython-old-sites/docs/library/stdtypes.html#boolean-values

Stackless Python: https://stackless.readthedocs.io/en/v3.6.9-slp/library/constants.html

PyPy: I couldn't find reference to bools in https://doc.pypy.org/en/latest/cpython_differences.html#object-identity-of-primitive-values-is-and-id, but I did attempt to quickly test it here: https://pypyjs.org/ See below:

sklearn/manifold/_t_sne.py

sklearn/manifold/tests/test_t_sne.py

Only new tests were changed. Other existing tests use docstrings, but were left unchanged.

I accidentally grouped an `sklearn.isotonic` enhancement in `sklearn.manifold` when fixing a merge conflict.

For previous wording, it was unclear what should be doing the squaring (TSNE or the user). New wording makes it explicit that TSNE is doing the squaring.

Some implementation details will need to be hashed out in 0.26, so add reminder.

`square_distance` -> `square_distances` pushed some lines > 79.

NicolasHug

thanks @joshuacwnewton , some last nitpicks and one question

doc/whats_new/v0.24.rst

sklearn/manifold/_t_sne.py

sklearn/manifold/tests/test_t_sne.py

NicolasHug

pre-approving since the remaining comments are easy to address. Thanks @joshuacwnewton !

`square_metric` -> `square_distances`

joshuacwnewton · 2020-07-11T16:06:49Z

Thanks much for taking the time to review, @NicolasHug! :D

jnothman · 2020-07-12T11:13:26Z

Thanks you @joshuacwnewton

…aring non-Euclidean distance metrics by default (scikit-learn#12401) (scikit-learn#17662)

joshuacwnewton added 9 commits June 22, 2020 10:06

Add new metric squaring behavior to TSNE

16141ea

Fix truth value check for string value cases

2659e3b

Remove one non-neg check when square_distance=True

662607e

Update non_positive test to ensure no squaring

8943229

Add tests for changes to square_distance

ead3235

Tests cover the following areas: * Invalid square_distance parameter selection * TSNE functioning with metric=Euclidean and square_distance=legacy * TSNE functioning with square_distance=True/False

Add docstring entry for squared_distance parameter

3f4fac3

Add test for square_distance FutureWarning cases

1c161d6

Revert change to non-neg check print statement

22dfd2d

github-actions bot added the module:manifold label Jun 23, 2020

joshuacwnewton added 3 commits June 23, 2020 08:45

Ignore FutureWarning in relevant TSNE tests

99f3928

Remove parameterization to speed up slow TSNE test

3fe6e04

Restore n_jobs param to pairwise_distances call

88340e6

flosincapite suggested changes Jun 23, 2020

View reviewed changes

sklearn/manifold/_t_sne.py Outdated Show resolved Hide resolved

sklearn/manifold/_t_sne.py Outdated Show resolved Hide resolved

sklearn/manifold/tests/test_t_sne.py Outdated Show resolved Hide resolved

sklearn/manifold/tests/test_t_sne.py Outdated Show resolved Hide resolved

joshuacwnewton changed the title ~~[WIP] ENH Allow non-Euclidean distance metrics to be squared in sklearn.manifold.TSNE~~ [MRG] ENH Allow non-Euclidean distance metrics to be squared in sklearn.manifold.TSNE (#12401) Jun 23, 2020

joshuacwnewton marked this pull request as ready for review June 23, 2020 17:55

joshuacwnewton added 2 commits June 24, 2020 08:44

Fix calls to pairwise_distances for Euclidean case

08d7b79

Euclidean handles squaring differently than other metrics. Euclidean is also slower for n_jobs > 1. So, the same call to pairwise_distances can't be used for every metric -- they have to be separated.

Write test to demonstrate squaring differences

5edf203

joshuacwnewton commented Jun 24, 2020

View reviewed changes

sklearn/manifold/tests/test_t_sne.py Outdated Show resolved Hide resolved

joshuacwnewton added 3 commits June 25, 2020 15:31

Replace random test data with clustered data

be27177

t-SNE relies on structure in input data. When random data is used, t-SNE becomes very sensitive to slight changes in the input. So, make_blobs is used to create data with structure instead.

Remove test for comparing squaring methods

bb1179a

No longer necessary now that source of instability has been identified.

Refactor tests to be more readable

16cb6de

joshuacwnewton force-pushed the 12401-add-TSNE-squaring branch from c7c588e to e1da68f Compare June 25, 2020 23:35

Parametrize square_dist test to improve coverage

6e5481f

joshuacwnewton force-pushed the 12401-add-TSNE-squaring branch from e1da68f to 6e5481f Compare June 25, 2020 23:37

Remove whitespace from existing test

79e7e22

I thought I had written this test, but it wasn't mine! So, this change shouldn't have happened in the first place.

Make '/" usage consistent for metric

6f2ed26

jeremiedbb reviewed Jul 2, 2020

View reviewed changes

sklearn/manifold/_t_sne.py Outdated Show resolved Hide resolved

sklearn/manifold/_t_sne.py Show resolved Hide resolved

joshuacwnewton added 3 commits July 2, 2020 08:43

Remove 'warn' from TSNE docstring

267d2a7

Explain deprecation in docstring/"What's New"

35eee7d

jeremiedbb approved these changes Jul 6, 2020

View reviewed changes

sklearn/manifold/_t_sne.py Outdated Show resolved Hide resolved

sklearn/manifold/_t_sne.py Outdated Show resolved Hide resolved

joshuacwnewton added 3 commits July 6, 2020 09:04

Update docstring to use proper deprecation format

dffdb5d

Remove last sentence in FutureWarning

beeb7ba

Merge upstream/master, fix conflict in What's New

cfaebe9

Another enhancement was added to the sklearn.manifold section, causing a merge conflict.

NicolasHug reviewed Jul 10, 2020

View reviewed changes

joshuacwnewton added 7 commits July 10, 2020 17:00

Replace test docstrings with comments

b33457d

Only new tests were changed. Other existing tests use docstrings, but were left unchanged.

Fix "What's New" manifold/isotonic header

7b201db

I accidentally grouped an `sklearn.isotonic` enhancement in `sklearn.manifold` when fixing a merge conflict.

Clarify wording of square_distance description

bb5cc55

For previous wording, it was unclear what should be doing the squaring (TSNE or the user). New wording makes it explicit that TSNE is doing the squaring.

Clarify meaning of "Euclidean n_jobs>1" comment

541dec4

Change square_distance to square_distances

7ca671b

Add TODO note re: deprecation of square_distances

918d9f2

Some implementation details will need to be hashed out in 0.26, so add reminder.

Fix line length linting issues

7ff25e1

`square_distance` -> `square_distances` pushed some lines > 79.

NicolasHug reviewed Jul 11, 2020

View reviewed changes

NicolasHug approved these changes Jul 11, 2020

View reviewed changes

joshuacwnewton added 4 commits July 11, 2020 08:33

Fix distance -> distances in What's New

826205f

Clarify deprecation plan in What's New

a748c97

Clarify deprecation plan in FutureWarning

a2b884b

Fix typo in test description comment

3b0e077

`square_metric` -> `square_distances`

joshuacwnewton changed the title ~~[MRG] ENH Allow non-Euclidean distance metrics to be squared in sklearn.manifold.TSNE (#12401)~~ [MRG] ENH Add parameter to sklearn.manifold.TSNE to transition to squaring non-Euclidean distance metrics by default (#12401) Jul 11, 2020

jnothman merged commit 89e49b6 into scikit-learn:master Jul 12, 2020
5 checks passed

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020

[MRG] ENH Add parameter to sklearn.manifold.TSNE to transition to squ…

54eb399

…aring non-Euclidean distance metrics by default (scikit-learn#12401) (scikit-learn#17662)

TomDLT mentioned this pull request Apr 1, 2021

FIX Approximate nearest neighbors in TSNE example #19809

Merged

[MRG] ENH Add parameter to sklearn.manifold.TSNE to transition to squaring non-Euclidean distance metrics by default (#12401) #17662

[MRG] ENH Add parameter to sklearn.manifold.TSNE to transition to squaring non-Euclidean distance metrics by default (#12401) #17662

joshuacwnewton commented Jun 23, 2020 •

edited

joshuacwnewton commented Jun 23, 2020 •

edited

joshuacwnewton commented Jun 23, 2020 •

edited

flosincapite left a comment

joshuacwnewton commented Jun 23, 2020 •

edited

This comment was marked as duplicate.

This comment was marked as duplicate.

joshuacwnewton Jun 24, 2020 •

edited

jeremiedbb Jun 24, 2020

joshuacwnewton Jun 24, 2020 •

edited

joshuacwnewton Jun 25, 2020 •

edited

joshuacwnewton Jun 25, 2020 •

edited

joshuacwnewton Jun 25, 2020

jnothman Jun 26, 2020

joshuacwnewton Jun 26, 2020

joshuacwnewton commented Jul 1, 2020

jeremiedbb left a comment

NicolasHug left a comment

NicolasHug Jul 10, 2020

joshuacwnewton Jul 10, 2020 •

edited

NicolasHug left a comment

NicolasHug left a comment

joshuacwnewton commented Jul 11, 2020

jnothman commented Jul 12, 2020

[MRG] ENH Add parameter to sklearn.manifold.TSNE to transition to squaring non-Euclidean distance metrics by default (#12401) #17662

[MRG] ENH Add parameter to sklearn.manifold.TSNE to transition to squaring non-Euclidean distance metrics by default (#12401) #17662

Conversation

joshuacwnewton commented Jun 23, 2020 • edited

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

joshuacwnewton commented Jun 23, 2020 • edited

joshuacwnewton commented Jun 23, 2020 • edited

flosincapite left a comment

Choose a reason for hiding this comment

joshuacwnewton commented Jun 23, 2020 • edited

This comment was marked as duplicate.

This comment was marked as duplicate.

joshuacwnewton Jun 24, 2020 • edited

Choose a reason for hiding this comment

jeremiedbb Jun 24, 2020

Choose a reason for hiding this comment

joshuacwnewton Jun 24, 2020 • edited

Choose a reason for hiding this comment

joshuacwnewton Jun 25, 2020 • edited

Choose a reason for hiding this comment

joshuacwnewton Jun 25, 2020 • edited

Choose a reason for hiding this comment

joshuacwnewton Jun 25, 2020

Choose a reason for hiding this comment

jnothman Jun 26, 2020

Choose a reason for hiding this comment

joshuacwnewton Jun 26, 2020

Choose a reason for hiding this comment

joshuacwnewton commented Jul 1, 2020

jeremiedbb left a comment

Choose a reason for hiding this comment

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug Jul 10, 2020

Choose a reason for hiding this comment

joshuacwnewton Jul 10, 2020 • edited

Choose a reason for hiding this comment

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug left a comment

Choose a reason for hiding this comment

joshuacwnewton commented Jul 11, 2020

jnothman commented Jul 12, 2020

joshuacwnewton commented Jun 23, 2020 •

edited

joshuacwnewton commented Jun 23, 2020 •

edited

joshuacwnewton commented Jun 23, 2020 •

edited

joshuacwnewton commented Jun 23, 2020 •

edited

joshuacwnewton Jun 24, 2020 •

edited

joshuacwnewton Jun 24, 2020 •

edited

joshuacwnewton Jun 25, 2020 •

edited

joshuacwnewton Jun 25, 2020 •

edited

joshuacwnewton Jul 10, 2020 •

edited