-
-
Notifications
You must be signed in to change notification settings - Fork 25.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MRG] t-SNE perplexity docstring update #13069
Conversation
sklearn/manifold/t_sne.py
Outdated
between 5 and 50. The choice is not extremely critical since t-SNE | ||
is quite insensitive to this parameter. | ||
between 5 and 50. While the choice is not extremely critical, results | ||
have been shown to change considerably across perplexity values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds a bit contradictory. Why not simply remove the entire sentence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is important for the user to know that results can change considerably across perplexity values. I could either remove the sentence or do something like:
Changing the perplexity value can significantly impact the result.
Either way, I think at the very least the sentence should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for your proposition
sklearn/manifold/t_sne.py
Outdated
@@ -507,8 +507,7 @@ class TSNE(BaseEstimator): | |||
The perplexity is related to the number of nearest neighbors that | |||
is used in other manifold learning algorithms. Larger datasets | |||
usually require a larger perplexity. Consider selecting a value | |||
between 5 and 50. While the choice is not extremely critical, results | |||
have been shown to change considerably across perplexity values. | |||
between 5 and 50. Different values can result in significanlty different results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NicolasHug the one I previously proposed made it seem like it was suggesting that changing the value from the default (30) would impact the results, hence this wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording is fine to me, but the checks are failing since the line is too long. Please wrap around 79 characters
Thanks @kjacks21 |
This reverts commit 97299ed.
This reverts commit 97299ed.
Reference Issues/PRs
Addresses #13040
What does this implement/fix? Explain your changes.
Updating the perplexity parameter docstring in order to show that the perplexity value does impact the results more than what the current docstring implied.