-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
API Deprecate n_iter
in favour of max_iter
for TSNE
#28471
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
Conversation
n_iter
in favour of max_iter
for TSNE
sklearn/manifold/_t_sne.py
Outdated
@@ -617,10 +618,22 @@ class TSNE(ClassNamePrefixFeaturesOutMixin, TransformerMixin, BaseEstimator): | |||
.. versionchanged:: 1.2 | |||
The default value changed to `"auto"`. | |||
|
|||
n_iter : int, default=1000 | |||
max_iter : int, default=None |
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'm not sure what the correct thing to do is here. From looking at other deprecations it seems that the docstring mentions the effective default value (I looked at make_sparse_spd_matrix
in sklearn/datasets/_samples_generator.py
), so like this:
max_iter : int, default=None | |
max_iter : int, default=1000 |
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.
Ah yes that is much better. I was following #25697 - I think it may be worth updating the docstring of BayesianRidge
and ARDRegression
even.
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.
Looks good to me, except for the comment about the docstring where I'm not sure and the linter's complaints need fixing.
@betatim thanks for the review! Do you think it is worthwhile changing |
So the common tests
|
Probably yes. Like you I think it would be nice for consistency, but it would mean more deprecation. However, I think a large fraction of people who use Not sure what to recommend. I think I'd implement the change and see what others say. |
In other estimators it's usually named |
Will leave |
I've amended the OP to 'closes' because I don't think |
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.
LGTM. Thanks @lucyleeow
Reference Issues/PRs
closes #7736
closes #7761 (supercedes)
What does this implement/fix? Explain your changes.
Amends
n_iter
in favour ofmax_iter
in TSNE because we have a convergence criterion and is more consistent with other estimators.Any other comments?
I think if we use
max_iter
we should probably amendn_iter_without_progress
to bemax_iter_without_progress
as well.