Skip to content

BUG fix SparseCoder to follow scikit-learn API#16346

Closed
glemaitre wants to merge 11 commits intoscikit-learn:masterfrom
glemaitre:is/16336
Closed

BUG fix SparseCoder to follow scikit-learn API#16346
glemaitre wants to merge 11 commits intoscikit-learn:masterfrom
glemaitre:is/16336

Conversation

@glemaitre
Copy link
Copy Markdown
Member

@glemaitre glemaitre commented Jan 31, 2020 •
edited
Loading

closes #16336

Refactor SparseCoder, DictionaryLearning, MiniBatchDictionaryLearning such that we follow properly our API.

There is a bit of boilerplate but I am not sure how to do better.

ogrisel
ogrisel reviewed Jan 31, 2020
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.

First batch of reviews:

TomDLT
TomDLT reviewed Feb 4, 2020
View reviewed changes
thomasjpfan
thomasjpfan reviewed Feb 12, 2020
View reviewed changes
thomasjpfan
thomasjpfan approved these changes Feb 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@NicolasHug
Copy link
Copy Markdown
Member

NicolasHug commented Feb 23, 2020

For SparseCoder, instead of removing the component_ attribute, would it be possible instead to set it in fit(), and raise a FutureWarning if it is accessed before the estimator is fitted?

@glemaitre
Copy link
Copy Markdown
Member Author

glemaitre commented Feb 23, 2020

SparseCoder does not require a call to fit indeed. This is a stateless estimator because we pass directly the dictionary. Requiring or forcing calling fit would be breaking backwards compatibility.

@glemaitre glemaitre mentioned this pull request Jun 24, 2020
Merged
@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Jun 25, 2020

Superseded by #17679.

@ogrisel ogrisel closed this Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ogrisel ogrisel ogrisel left review comments

@thomasjpfan thomasjpfan thomasjpfan approved these changes

+1 more reviewer

@TomDLT TomDLT TomDLT left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module:decomposition

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SparseCoder does not follow the scikit-learn API and will break in 0.24

5 participants

@glemaitre @NicolasHug @ogrisel @thomasjpfan @TomDLT