Skip to content

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

Merged
merged 10 commits into from
Jun 27, 2022

Conversation

glemaitre
Copy link
Member

This PR is deprecating n_iter and introducing max_iter, tol, and max_no_improvement parameters to be consistent with MiniBatchDictionaryLearning.

We should do this step in version 1.1 since some code will have to be removed and n_iter would not be available in MiniBatchDictionaryLearning required by the MiniBatchSparsePCA.
We should therefore backport this fix in 1.1.2.

@jeremiedbb jeremiedbb added this to the 1.1.2 milestone Jun 22, 2022
Copy link
Member

@jeremiedbb jeremiedbb left a 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.

glemaitre and others added 2 commits June 22, 2022 14:45
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
@glemaitre
Copy link
Member Author

I also move the deprecation warning raise in MiniBatchDictionaryLearning such that it is triggered when one both sets n_iter and max_iter. It is documented that we ignore n_iter in this case which is fine. However, we should raise the warning such that our users know that they have to remove the n_iter from the constructor.

test_spca_early_stopping
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

glemaitre and others added 2 commits June 22, 2022 16:54
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
Copy link
Member

@ogrisel ogrisel Jun 24, 2022

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.

Copy link
Member

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.

Copy link
Member

@ogrisel ogrisel left a 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.

@ogrisel ogrisel modified the milestones: 1.1.2, 1.2 Jun 24, 2022
@glemaitre glemaitre merged commit 06834fc into scikit-learn:main Jun 27, 2022
ogrisel added a commit to ogrisel/scikit-learn that referenced this pull request Jul 11, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants