Skip to content

MNT simple deprecations and removals for 0.21 #12238

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
Merged
merged 12 commits into from
Oct 11, 2018
Merged

MNT simple deprecations and removals for 0.21 #12238

merged 12 commits into from
Oct 11, 2018

Conversation

amueller
Copy link
Member

@amueller amueller commented Oct 1, 2018

Part of #11992.
These were all the things that seemed pretty straight-forward. It's actually a bit bulky but should still be easy to review, hopefully.

@amueller amueller mentioned this pull request Oct 1, 2018
Closed
@sklearn-lgtm
Copy link

This pull request introduces 17 alerts and fixes 2 when merging c56c2af into 59b15c5 - view on LGTM.com

new alerts:

  • 16 for Unused import
  • 1 for Explicit export is not defined

fixed alerts:

  • 2 for Unused local variable

Comment posted by LGTM.com

@sklearn-lgtm
Copy link

This pull request introduces 17 alerts and fixes 2 when merging cd48d6d into 59b15c5 - view on LGTM.com

new alerts:

  • 16 for Unused import
  • 1 for Explicit export is not defined

fixed alerts:

  • 2 for Unused local variable

Comment posted by LGTM.com

@sklearn-lgtm
Copy link

This pull request fixes 2 alerts when merging 5069dcf into 59b15c5 - view on LGTM.com

fixed alerts:

  • 2 for Unused local variable

Comment posted by LGTM.com

@sklearn-lgtm
Copy link

This pull request fixes 2 alerts when merging cfa9216 into 7166cd5 - view on LGTM.com

fixed alerts:

  • 2 for Unused local variable

Comment posted by LGTM.com

@sklearn-lgtm
Copy link

This pull request fixes 2 alerts when merging 45289e8 into dfd009d - view on LGTM.com

fixed alerts:

  • 2 for Unused local variable

Comment posted by LGTM.com

rth
rth approved these changes Oct 2, 2018
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.

LGTM, nice to remove all this code! Only checked what's included though, if anything is missing it could be worth doing a follow up PR.

@amueller
Copy link
Member Author

amueller commented Oct 2, 2018

Yes, it's a bit hard to check for completeness - this is also not complete in the sense that it misses (at least) 4 major things (#12239 #12240 #12241 + non_negative) that looked slightly non-trivial.
I think doing this in increments makes sense.

@sklearn-lgtm
Copy link

This pull request fixes 2 alerts when merging 3afc42b into dfd009d - view on LGTM.com

fixed alerts:

  • 2 for Unused local variable

Comment posted by LGTM.com

@amueller amueller mentioned this pull request Oct 2, 2018
Merged
@sklearn-lgtm
Copy link

This pull request fixes 2 alerts when merging 84d6cf3 into 63e5ae6 - view on LGTM.com

fixed alerts:

  • 2 for Unused local variable

Comment posted by LGTM.com

jnothman
jnothman approved these changes Oct 8, 2018
View reviewed changes
Copy link
Member

@jnothman jnothman 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 not checking that these are unused/undocumented. Let's hope that was done at time of deprecation. The code changes seem sound.

@jnothman jnothman changed the title simple deprecations and removals for 0.21 MNT simple deprecations and removals for 0.21 Oct 8, 2018
@amueller
Copy link
Member Author

should I merge now? Should be fine, right? Or wait for 0.20.1? I don't think it should conflict too much?

@jnothman
Copy link
Member

I doubt it should affect 0.20.1.

(Though we do need to get onto making that release happen sooner rather than later.)

@amueller amueller merged commit 0f94f29 into scikit-learn:master Oct 11, 2018
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

@eamanu eamanu eamanu left review comments

@jnothman jnothman jnothman 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.
5 participants
@amueller @sklearn-lgtm @jnothman @rth @eamanu