ENH Improve initialization and learning rate in t-SNE

[dkobak]

This implements suggestions from #18018 (see there for some discussion):

1. Clarifies the documentation about the learning_rate (different from all other implementations by a factor of 4).
2. Scales PCA initialization to have the same std as the random initialization. (Update: only issues future warning for now. Would change in v1.2.)
3. Issues future warning that PCA initialization will become default in v1.2.
4. Implements learning_rate='auto' that scales the learning rate with the sample size.
5. Issues future warning that learning_rate='auto' will become default in v1.2.

I would still have to implement unit tests for future warnings (haven't done it before) and add the changes to whats_new (not quite sure which of the above changes need to be mentioned there). But I'd like to get some feedback from the core developers about whether these suggested changes are all fine. @TomDLT @ogrisel

Update: tests added, changes added.

[TomDLT]

Thanks for the pull-request!

[dkobak]

@TomDLT I fixed and added t-SNE tests and they seem to be working fine, but something is failing in doctest:

================================== FAILURES ===================================
____________________ [doctest] sklearn.manifold._t_sne.TSNE ____________________
...
UNEXPECTED EXCEPTION: FutureWarning("The default initialization in TSNE will change from 'random' to 'pca' in 1.2.")

and I don't know where to fix it :-/

[dkobak]

@thomasjpfan Thanks a lot for reviewing! I pushed your suggestions and added TODO comments and docstrings as you suggested everywhere else too.

This PR is deprecating three things: two defaults value and the PCA SD change. I suggest removing the PCA SD change for now and keep the deprecation for learning_rate and init. In the user guide (doc/modules/manifold.rst), we need to describe learning_rate='auto' with references. In a future PR we can update the user guide for the PCA SD change and add the warning + tests.

Sorry, I am not sure I understand the rationale here. I already have everything regarding PCA SD implemented here, what's the point of taking it out? Also, I would be uncomfortable setting PCA init as default if it does not get scaled to correct SD. To be honest, I think these changes should go together.

Update: I mean, the SD change is currently implemented but commented out, because it should only go live in version 1.2. Not sure what's the better way to do it? I think the future warning should be happening already in this PR.

             # X_embedded = X_embedded / np.std(X_embedded[:, 0]) * 1e-4

In the user guide (doc/modules/manifold.rst), we need to describe learning_rate='auto' with references.

This I can do. Update: done!

Also, what do you think about this suggested change (not yet implemented in this PR):

Oh, there is something I forgot to mention in the original issue: after implementing the learning_rate = n/12 heuristic in openTSNE and FIt-SNE we realized that 750 iterations is enough for all practical purposes and made n_iter=750 the default over there in both implementations (see a LONG discussion here KlugerLab/FIt-SNE#88).

So we could also adopt the same convention here, cutting down the number of default iterations from 1000 to 750. This of course would need to go through a deprecation cycle, together with the learning_rate='auto'. What do you think?

In case you think it's a good idea, I am wondering if the deprecation cycle needs to be implemented via n_iter='warn'. Given that this is tied to the learning_rate change, can the learning rate FutureWarning mention that the n_iter will change to 750 together with the future learning rate change? Without an additional n_iter future warning?

PS. Not sure why the milestone check is not suddenly failing...

[dkobak]

@thomasjpfan Could you clarify what you meant by "removing the PCA SD change for now"? See also my comment above for more considerations. Everything is fixed btw. Cheers!

[thomasjpfan]

Could you clarify what you meant by "removing the PCA SD change for now"? See also my comment above for more considerations. Everything is fixed btw. Cheers!

What I meant was that the commented out PCA change can be its own pull request. But looking at this again, I am okay with leaving it in.

I can see that the depredations in this PR are all related, but they could be done with three separate PRs. This makes it easier to review and helps with merging faster. In general, a PR with bigger scope increases the chances of something blocking it from merging.

So we could also adopt the same convention here, cutting down the number of default iterations from 1000 to 750. This of course would need to go through a deprecation cycle, together with the learning_rate='auto'. What do you think?

We can work on this in a follow up PR. This PR is already a net improvement as is.

As for the review, I left comments about using pytest.mark.filterwarnings instead of ignore_warnings that applies to all the tests. Otherwise this looks good to go.

[thomasjpfan]

Can we move this references into the References section below and link it here?

scikit-learn/sklearn/manifold/_t_sne.py

Line 638 in 962bd9a

References

[dkobak]

Makes sense, fixed.

[dkobak]

@thomasjpfan Thanks a lot! I changed the handling of the future warnings. All checks pass.

We can work on this in a follow up PR. This PR is already a net improvement as is.

My suggestion is simply to replace

        if self.learning_rate == 'warn':
            # See issue #18018
            warnings.warn("The default learning rate in TSNE will change "
                          "from 200.0 to 'auto' in 1.2.", FutureWarning)

with

        if self.learning_rate == 'warn':
            # TODO: Change the n_iter to 750 in 1.2.
            # See issue #18018
            warnings.warn("The default learning rate in TSNE will change "
                          "from 200.0 to 'auto' in 1.2. At the same time, the default 
                          "number of iterations will decrease from 1000 to 750",
                          FutureWarning)

I think this does not deserve its own PR... There would nothing else to do really at this point. Or what do you think?

[TomDLT]

I am not fully convinced that changing the default number of iteration from 1000 to 750 is necessary. It would probably benefit from a dedicated discussion in a small separate PR.

[dkobak]

I am not fully convinced that changing the default number of iteration from 1000 to 750 is necessary. It would probably benefit from a dedicated discussion in a small separate PR.

Fair enough. This change would not have any other consequences apart from decreasing the runtime by 25%. I'm fine merging this PR without this change if you guys prefer that.

[thomasjpfan]

Thank you for working on this issue @dkobak !

LGTM