Skip to content

DEPR deprecate n_iter in MiniBatchSparsePCA#23726

Merged
glemaitre merged 10 commits intoscikit-learn:mainfrom
glemaitre:depr/n_iter_sparse_pca
Jun 27, 2022
Merged

DEPR deprecate n_iter in MiniBatchSparsePCA#23726
glemaitre merged 10 commits intoscikit-learn:mainfrom
glemaitre:depr/n_iter_sparse_pca

Conversation

@glemaitre
Copy link
Copy Markdown
Member

@glemaitre glemaitre commented Jun 22, 2022

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
jeremiedbb
jeremiedbb reviewed Jun 22, 2022
View reviewed changes
Copy link
Copy Markdown
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
@glemaitre @jeremiedbb
Apply suggestions from code review
78270fe 
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
@glemaitre
be sure to warn when was set
4354a2a
@glemaitre
Copy link
Copy Markdown
Member Author

glemaitre commented Jun 22, 2022

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.

jeremiedbb
jeremiedbb reviewed Jun 22, 2022
View reviewed changes
@jeremiedbb
just to be sure :) [all random seeds]
7dc6588 
test_spca_early_stopping
jeremiedbb
jeremiedbb approved these changes Jun 22, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb left a comment •
edited
Loading

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
@glemaitre @jeremiedbb
Apply suggestions from code review
3378ffb 
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
@glemaitre
Update _dict_learning.py
158c98e
ogrisel
ogrisel reviewed Jun 24, 2022
View reviewed changes
sklearn/decomposition/_sparse_pca.py
To disable convergence detection based on cost function, set
`max_no_improvement` to `None`.

.. versionadded:: 1.1
Copy link
Copy Markdown
Member

@ogrisel ogrisel Jun 24, 2022 •
edited
Loading

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
Copy Markdown
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.

I pushed 6ea3f39 to do that.

ogrisel
ogrisel approved these changes Jun 24, 2022
View reviewed changes
Copy link
Copy Markdown
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
@glemaitre @jeremiedbb @ogrisel
DEPR deprecate n_iter in MiniBatchSparsePCA (scikit-learn#23726)
67fb7fa 
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

Reviewers

@ogrisel ogrisel ogrisel approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees

No one assigned

Labels

module:decomposition

Projects

None yet

Milestone

1.2

Development

Successfully merging this pull request may close these issues.

3 participants

@glemaitre @ogrisel @jeremiedbb