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.

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

[MRG+2] remove modification of warning registry for no reason #9569

Merged
merged 1 commit into from Sep 8, 2017

Conversation

Projects
None yet
7 participants
@amueller
Member

amueller commented Aug 16, 2017

Fixes #9560. Fixes #2755. Fixes #7346.

@amueller

This comment has been minimized.

Member

amueller commented Aug 16, 2017

Arguably this shouldn't even be a getattr.

@jnothman

This comment has been minimized.

Member

jnothman commented Aug 16, 2017

I think this is a worthwhile improvement, but it may result in warnings being issued with custom estimator code where they had not been.

Let's get someone else's opinion, be bold and merge as it should not functionally break anyone's code, they'll just be forced to hide the warning or do any validation in fit and other good practice...

@amueller

This comment has been minimized.

Member

amueller commented Aug 17, 2017

I agree. If they use a property, I think they are unlikely to pass check_estimator.

I let my student run a bigquery search to see how often people deprecate stuff at all (by searching for BaseEstimator and DeprecationWarning or @deprecated, which clearly has false negatives because people might not inherit, but should give us some ideas).
I think the number of people that deprecate at all is probably pretty small.

@jnothman

This comment has been minimized.

Member

jnothman commented Aug 17, 2017

@jnothman

This comment has been minimized.

Member

jnothman commented Aug 20, 2017

Opinion, @ogrisel?

@amueller

This comment has been minimized.

Member

amueller commented Aug 21, 2017

Btw, I had my student check github for code including BaseEstimator and deprecated/DeprecationWarning. Of the 60 (!) files that has both, most were code copied 1:1 from sklearn, or code in tensorflow. No evidence that anyone (that inherits from BaseEstimator) does anything like what the test was testing.

@jnothman

This comment has been minimized.

Member

jnothman commented Aug 21, 2017

Seeing as you're removing all of the warnings mess from get_params, I've added the other two issues to those this PR closes.

LGTM

@jnothman jnothman changed the title from RFC? remove modification of warning registry for no reason to [MRG+1] remove modification of warning registry for no reason Aug 21, 2017

@amueller

This comment has been minimized.

Member

amueller commented Aug 29, 2017

@GaelVaroquaux is with us again and might have opinions.

warnings.filters.pop(0)
# XXX: should we rather test if instance of estimator?
value = getattr(self, key, None)

This comment has been minimized.

@lesteve

lesteve Aug 30, 2017

Member

Just curious, is there any reason why getattr(self, key) is not enough?

This comment has been minimized.

@jnothman

jnothman Aug 30, 2017

Member

Probably not, but best not break stuff.

This comment has been minimized.

@lesteve

lesteve Aug 30, 2017

Member

Fine.

@lesteve

This comment has been minimized.

Member

lesteve commented Aug 30, 2017

I have looked carefully at the related discussions, this change seems fine and I would be in favour of merging.

To help other reviewers, I reckon this comment from @amueller is quite a nice concise explanation about the test removal.

@lesteve

This comment has been minimized.

Member

lesteve commented Sep 8, 2017

@ogrisel maybe you can have a look at this one?

@GaelVaroquaux GaelVaroquaux changed the title from [MRG+1] remove modification of warning registry for no reason to [MRG+2] remove modification of warning registry for no reason Sep 8, 2017

@GaelVaroquaux

This comment has been minimized.

Member

GaelVaroquaux commented Sep 8, 2017

I am 👍 for this PR (having read the carefully study of @amueller, thanks a lot for the awesome work).

I would suggest to merge, unless @ogrisel has comments.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Sep 8, 2017

+1, let's keep this code as simple as possible.

@ogrisel ogrisel merged commit 156a1f3 into scikit-learn:master Sep 8, 2017

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing e1fb03c...8d640b9
Details
codecov/project 96.17% (-0.01%) compared to e1fb03c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@mrocklin

This comment has been minimized.

mrocklin commented Sep 8, 2017

Thanks @amueller

@amueller

This comment has been minimized.

Member

amueller commented Sep 8, 2017

thanks for the reviews y'all :)

@jnothman

This comment has been minimized.

Member

jnothman commented Nov 13, 2017

We didn't include this in 0.19.1... for no good reason. And here #10129 proposing we fix this in 0.18.X!! For the sake of distributed users, is it worth creating bug-fix releases with this??

@lesteve

This comment has been minimized.

Member

lesteve commented Nov 14, 2017

What is the advantage of having a 0.18.3 release rather than a 0.19.2 release? In the latter case, the nose dependency removal could be added as well.

@jnothman

This comment has been minimized.

Member

jnothman commented Nov 14, 2017

@lesteve

This comment has been minimized.

Member

lesteve commented Nov 14, 2017

I would not read too much in #10129 targetting 0.18.X to be perfectly honest and I did not hear an argument for a 0.18.3 release so I am not really in favour of 0.18.3.

Maybe I am wrong but I thought we were only doing bug fixes in the latest release and I would try to keep it this way.

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

k-dominik added a commit to k-dominik/ilastik that referenced this pull request Aug 8, 2018

Monkey patched sklearn
this sort of fixes ilastik#1149

However, this is just a monkey patch, and as of sklearn 0.20 it will be
included in sklearn itself.

reference: scikit-learn/scikit-learn#9569

@k-dominik k-dominik referenced this pull request Aug 8, 2018

Merged

Monkey patched sklearn #1810

1 of 1 task complete

k-dominik added a commit to k-dominik/ilastik that referenced this pull request Aug 8, 2018

Monkey patched sklearn
this sort of fixes ilastik#1149

However, this is just a monkey patch, and as of sklearn 0.20 it will be
included in sklearn itself.

reference: scikit-learn/scikit-learn#9569

paulhfu added a commit to paulhfu/ilastik that referenced this pull request Aug 20, 2018

Monkey patched sklearn
this sort of fixes ilastik#1149

However, this is just a monkey patch, and as of sklearn 0.20 it will be
included in sklearn itself.

reference: scikit-learn/scikit-learn#9569
@michcio1234

This comment has been minimized.

michcio1234 commented Nov 2, 2018

Is this included in 0.20.0? I'm suffering from #7346...

@jnothman

This comment has been minimized.

Member

jnothman commented Nov 5, 2018

Yes, @michcio1234, it is included in 0.20.0. If you have 0.20.0 installed properly, it is impossible that you are suffering from #7346 with a similar traceback. Please open a new issue with full details.

@michcio1234

This comment has been minimized.

michcio1234 commented Nov 5, 2018

@jnothman thanks, I didn't notice that my CI server had 0.19. Installing 0.20 fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment