8000 Review DeprecationWarnings and FutureWarnings in raise tests for the 0.20 release · Issue #11252 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Review DeprecationWarnings and FutureWarnings in raise tests for the 0.20 release #11252

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
Closed
ogrisel opened this issue Jun 13, 2018 · 6 comments
Closed

Review DeprecationWarnings and FutureWarnings in raise tests for the 0.20 release #11252

ogrisel opened this issue Jun 13, 2018 · 6 comments
Labels
good first issue Easy with clear instructions to resolve help wanted
Milestone
0.20

Comments

@ogrisel
Copy link
Member
ogrisel commented Jun 13, 2018 •
edited
Loading

Prior to the release, we need to conduct an audit specifically on DeprecationWarning and FutureWarnings raised in the tests (including doctests from the doc). Then on a case by case basis we need to decide for each occurrence whether to:

  • change the test specifically to check that the deprecated feature actually raise the warning but still work the way it used to work in 0.19.1
  • change the test to stop using the deprecated feature if the ultimate goal of the test is not to test the backward compatibility of the deprecated feature.
  • ignore the warnings if we have a specific reason to do so: for instance in test_common.py it's ok to blindly ignore the warnings because the automated estimator introspection lookup will trigger them.

In any case, we should never blindly ignore the warning. In the very few cases where there is a legetimate reason to catch the warning (as in test_common), we need to at least write why it's safe to ignore this specific warning occurrence in an inline comment in the source code of the test.
@ogrisel ogrisel added this to the 0.20 milestone Jun 13, 2018
@glemaitre glemaitre mentioned this issue Jun 13, 2018
Closed
@ogrisel ogrisel mentioned this issue 8000 Jun 13, 2018
Merged
@ogrisel ogrisel changed the title Review DeprecationWarnings in raise tests for the 0.20 release Review DeprecationWarnings and FutureWarnings in raise tests for the 0.20 release Jun 13, 2018
@ogrisel
Copy link
Member Author
ogrisel commented Jun 14, 2018

We should finish and merge #9570 prior to conducting this audit.

@ogrisel ogrisel added good first issue Easy with clear instructions to resolve help wanted labels Jul 5, 2018
@ogrisel
Copy link
Member Author
ogrisel commented Jul 5, 2018

This can be done progressively, with several PR, one test module at a time.

@rth
Copy link
Member
rth commented Jul 5, 2018

Related: #11431 aims to ensure that there are no warnings at module import time.

@amueller
Copy link
Member

ping @janvanrijn

@amueller amueller mentioned this issue Jul 16, 2018
Merged
amueller added a commit that referenced this issue Jul 17, 2018
@amueller
raise DeprecationWarnings and FutureWarnings as errors (#11570)
8e2c2aa 
Towards #11252.
In the end we'd like to make these errors so we can keep this cleaner in the future.
@amueller
Copy link
Member

fixed in #11570 I would argue ;)

@amueller
Copy link
Member

fixed in #11570 I would argue ;)

@massich massich mentioned this issue Aug 6, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Easy with clear instructions to resolve help wanted
Projects
None yet
Milestone
0.20
Development

No branches or pull requests
3 participants
@ogrisel @amueller @rth
0