Skip to content
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.

Sign up for GitHub

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

Jump to bottom

MNT deprecate outputs_2d_ attribute of dummy estimators #14933

Merged
merged 6 commits into from Sep 15, 2019
Merged

MNT deprecate outputs_2d_ attribute of dummy estimators #14933

merged 6 commits into from Sep 15, 2019

Conversation

NicolasHug
Copy link
Member

It is equivalent to n_outputs_ != 1.

closes #14801

@NicolasHug NicolasHug mentioned this pull request Sep 9, 2019
Closed
adrinjalali
adrinjalali approved these changes Sep 13, 2019
View reviewed changes
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

LGTM, we just need a whats new entry for it.

@NicolasHug
Copy link
Member Author

I added an entry.

I feel like we need a different kind of entry for these deprecations which are very minor and do not impact as much as other deprecations. The attribute wasn't ever documented, I doubt people were relying on it.

@adrinjalali
Copy link
Member

I added an entry.

I feel like we need a different kind of entry for these deprecations which are very minor and do not impact as much as other deprecations. The attribute wasn't ever documented, I doubt people were relying on it.

We need a separate deprecated section IMO, for all the things we're deprecating. But that can be done in a separate PR.

@thomasjpfan
Copy link
Member

Lets do the new "deprecated attributes" section now?

@thomasjpfan
Copy link
Member

Let do it in another PR, but it needs to be soon. The more attribute we deprecate, the more we have to move things around.

thomasjpfan
thomasjpfan approved these changes Sep 13, 2019
View reviewed changes
doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
rth
rth approved these changes Sep 13, 2019
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Minor comment, LGTM otherwise.

sklearn/dummy.py Outdated Show resolved Hide resolved
@rth
Copy link
Member

rth commented Sep 13, 2019

Let do it in another PR, but it needs to be soon. The more attribute we deprecate, the more we have to move things around.

Can we automate the generation of that? Looks like we are going to deprecate a lot of things. If we had an automated way that would also simplify removals. Though I'm not sure if it's worth spending time on such a system.

@thomasjpfan
Copy link
Member

Though I'm not sure if it's worth spending time on such a system.

I would not want to...make it easier to deprecate things ;)

@rth
Copy link
Member

rth commented Sep 13, 2019

I would not want to...make it easier to deprecate things ;)

Exactly, not sure where we'll stop otherwise :)

qinhanmin2014
qinhanmin2014 approved these changes Sep 15, 2019
View reviewed changes
doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
@NicolasHug
Copy link
Member Author

Thanks for the reviews, I addressed the comments

@NicolasHug
Copy link
Member Author

+4 so I'll merge

@NicolasHug NicolasHug changed the title [MRG] MNT deprecate outputs_2d_ attribute of dummy estimators MNT deprecate outputs_2d_ attribute of dummy estimators Sep 15, 2019
@NicolasHug NicolasHug merged commit 84717a9 into scikit-learn:master Sep 15, 2019
@NicolasHug NicolasHug deleted the outputs_2d_dep_dummy branch September 15, 2019 22:09
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 16, 2019
@NicolasHug @glemaitre
MNT deprecate outputs_2d_ attribute of dummy estimators (scikit-learn…
9b65ed7 
…#14933)
@marenwestermann marenwestermann mentioned this pull request Nov 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rth rth rth approved these changes

@adrinjalali adrinjalali adrinjalali approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@qinhanmin2014 qinhanmin2014 qinhanmin2014 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@NicolasHug @adrinjalali @thomasjpfan @rth @qinhanmin2014