-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DEPR deprecate n_iter in MiniBatchSparsePCA #23726
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
DEPR deprecate n_iter in MiniBatchSparsePCA #23726
Conversation
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 fine with considering it a bug in the deprecation cycle of MiniBatchDictionaryLearning
, thus targeting 1.1 rather than 1.2 to keep both in sync.
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
I also move the deprecation warning raise in |
test_spca_early_stopping
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
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
To disable convergence detection based on cost function, set | ||
`max_no_improvement` to `None`. | ||
|
||
.. versionadded:: 1.1 |
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.
In retrospect it's a bit weird to introduce a new parameter in a bugfix release.
Maybe we should instead move this PR to target 1.2 and update the previous deprecation message for the MB sparse PCA to 1.4 instead of 1.3 so that both the MB dict learning and sparse PCA variant are actually removed in 1.4 at the same time to ease maintenance.
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 pushed 6ea3f39 to do that.
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.
Assuming CI is green, LGTM.
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com> Co-authored-by: jeremie du boisberranger <jeremiedbb@yahoo.fr> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
This PR is deprecating
n_iter
and introducingmax_iter
,tol
, andmax_no_improvement
parameters to be consistent withMiniBatchDictionaryLearning
.We should do this step in version 1.1 since some code will have to be removed and
n_iter
would not be available inMiniBatchDictionaryLearning
required by theMiniBatchSparsePCA
.We should therefore backport this fix in 1.1.2.