Skip to content

[MRG] Simplify super() calls#12812

Merged
rth merged 14 commits intoscikit-learn:masterfrom
JarnoRFB:simpler-super-calls-for-dummies
Jan 10, 2019
Merged

[MRG] Simplify super() calls#12812
rth merged 14 commits intoscikit-learn:masterfrom
JarnoRFB:simpler-super-calls-for-dummies

Conversation

@JarnoRFB
Copy link
Copy Markdown
Contributor

@JarnoRFB JarnoRFB commented Dec 18, 2018

Now that Python 2 is removed from the CI we can call super without
additional arguments.

@JarnoRFB
[MRG] Simplify super() calls
df9ca33 
Now that Python 2 is removed from the CI we can call super without
additional arguments.
@adrinjalali
Copy link
Copy Markdown
Member

adrinjalali commented Dec 18, 2018

  1. It'll probably be better to wait for these changes until MRG Drop legacy python / remove six dependencies #12639 is merged.
  2. If we decide to change the super() calls, we should do it everywhere I guess. And I'm not sure how much conflict with other PRs it'll cause.

@JarnoRFB
Copy link
Copy Markdown
Contributor Author

JarnoRFB commented Dec 18, 2018

Fair enough. I just had a small PR on the dummies a few month ago, where we discussed this change and decided to wait until Python 2 is removed from the CI. So I thought it might be time now. In #11991 super calls are identified as something that should be changed. I would also be happy to remove all them!

@adrinjalali
Copy link
Copy Markdown
Member

adrinjalali commented Dec 18, 2018

Cool, then we can wait until #12639 is merged and then you can go ahead with the super calls which remain.

@JarnoRFB JarnoRFB changed the title [MRG] Simplify super() calls for dummies [WIP] Simplify super() calls Dec 30, 2018
@JarnoRFB JarnoRFB mentioned this pull request Dec 30, 2018
Merged
@JarnoRFB JarnoRFB changed the title [WIP] Simplify super() calls [MRG] Simplify super() calls Jan 3, 2019
rth
rth reviewed Jan 3, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Changing this in __init__ of estimators sounds reasonable, but I wonder if making the change in base classes (see below) is really equivalent in functionality.

@JarnoRFB
Copy link
Copy Markdown
Contributor Author

JarnoRFB commented Jan 9, 2019

Issues with the test are resolved. From my side this would be ready to merge.

@rth rth mentioned this pull request Jan 10, 2019
Closed
rth
rth reviewed Jan 10, 2019
View reviewed changes
jnothman
jnothman reviewed Jan 10, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Reverting changes to externals is the only blocker for merge here. Otherwise LGTM

amueller
amueller approved these changes Jan 10, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@amueller amueller left a comment

Choose a reason for hiding this comment

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

lgtm

rth
rth approved these changes Jan 10, 2019
View reviewed changes
Copy link
Copy Markdown
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, thanks @JarnoRFB !

@rth rth merged commit d300f40 into scikit-learn:master Jan 10, 2019
@JarnoRFB JarnoRFB deleted the simpler-super-calls-for-dummies branch January 11, 2019 11:06
@rth rth mentioned this pull request Jan 16, 2019
Merged
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
@JarnoRFB
MAINT Simplify super() calls (scikit-learn#12812)
129c28f
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "MAINT Simplify super() calls (scikit-learn#12812)"
5f0137f 
This reverts commit 129c28f.
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "MAINT Simplify super() calls (scikit-learn#12812)"
2921026 
This reverts commit 129c28f.
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@JarnoRFB @koenvandevelde
MAINT Simplify super() calls (scikit-learn#12812)
275325a
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Nov 1, 2019
@ivannz
Simplified super() calls according to scikit-learn#12812
d704337
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Nov 1, 2019
@ivannz
Simplified super() calls according to scikit-learn#12812
c61969d
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Jan 10, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
55e08ae
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Jan 30, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
4b30829
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Jan 30, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
72dd1b0
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Feb 27, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
f1c2b56
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Mar 12, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
9916a64
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Apr 17, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
8ea90de
ivannz added a commit to ivannz/scikit-learn that referenced this pull request May 3, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
4639f8b
ivannz added a commit to ivannz/scikit-learn that referenced this pull request May 30, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
eb1504a
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Jul 5, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
79815f3
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Jul 5, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
946c6ce
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Aug 5, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
212f826
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Oct 15, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
b8753d6
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Nov 28, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
469edd6
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Dec 12, 2020
@ivannz
Simplified super() calls according to scikit-learn#12812
6075e59
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Feb 25, 2021
@ivannz
Simplified super() calls according to scikit-learn#12812
f4a0623
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Feb 25, 2021
@ivannz
Simplified super() calls according to scikit-learn#12812
3cfe8d6
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Mar 27, 2021
@ivannz
Simplified super() calls according to scikit-learn#12812
e8bb663
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Jun 15, 2021
@ivannz
Simplified super() calls according to scikit-learn#12812
26ba1ca
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Jul 23, 2021
@ivannz
Simplified super() calls according to scikit-learn#12812
030bcd6
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Nov 10, 2021
@ivannz
Simplified super() calls according to scikit-learn#12812
c8e6602
ivannz added a commit to ivannz/scikit-learn that referenced this pull request May 15, 2022
@ivannz
Simplified super() calls according to scikit-learn#12812
a6471ba
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Jun 14, 2022
@ivannz
Simplified super() calls according to scikit-learn#12812
e8e78f6
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Aug 29, 2022
@ivannz
Simplified super() calls according to scikit-learn#12812
aba1039
ivannz added a commit to ivannz/scikit-learn that referenced this pull request Sep 5, 2022
@ivannz
Simplified super() calls according to scikit-learn#12812
8c60b69
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnothman jnothman jnothman left review comments

@amueller amueller amueller approved these changes

+1 more reviewer

@rth rth rth approved these changes

Reviewers whose approvals may not affect merge requirements

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

@JarnoRFB @adrinjalali @jnothman @amueller @rth