Skip to content

raise DeprecationWarnings and FutureWarnings as errors #11570

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

Merged
merged 41 commits into from
Jul 17, 2018

Conversation

amueller
Copy link
Member

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 Author

this should be everything except model_selection and isolation forest and bagging which uses Imputer (these two have separate issues).

@amueller amueller added this to the 0.20 milestone Jul 16, 2018
@amueller
Copy link
Member Author

merged #11593 into this so tests may pass.

@@ -473,6 +473,7 @@ def test_lars_path_readonly_data():
_lars_path_residues(X_train, y_train, X_test, y_test, copy=False)


@ignore_warnings(DeprecationWarning) # positive deprecated, remove in 0.22
Copy link
Member

@rth rth Jul 17, 2018

Choose a reason for hiding this comment

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

This will skip the test due to #11594 (also applies to all other tests)

@@ -1,20 +1,21 @@
"""Test the search module"""

from collections import Iterable, Sized
Copy link
Member

Choose a reason for hiding this comment

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

from sklearn.utils.fixes import _Iterable as Iterable, _Sized as Sized

otherwise we get collections ABC warning in Python 3.7

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, right. not sure why @janvanrijn changed that.

Copy link
Contributor

Choose a reason for hiding this comment

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

My code is an (old) version of master. I did not intentionally change that

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, that makes sense.


import numpy as np
import scipy.sparse as sp
import pytest

from sklearn.utils.fixes import sp_version
from sklearn.utils.fixes import PY3_OR_LATER
from sklearn.utils.fixes import _Iterable as Iterable, _Sized as Sized
Copy link
Member

Choose a reason for hiding this comment

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

Why? That's necessary to avoid warnings in Python 3.7

@rth
Copy link
Member

rth commented Jul 17, 2018

I think we need need to test Python 3.7 in Travis and not in a cron job (#11409). Otherwise this would have failed on Python 3.7 but we won't know because we are not testing for it..

@amueller
Copy link
Member Author

@rth we should but that seems unrelated ;)

@rth
Copy link
Member

rth commented Jul 17, 2018

@amueller you are right.

@massich
Copy link
Contributor

massich commented Jul 17, 2018

Apart from what @lesteve had catch in #11594 the rest LGTM.

before 0.20 is not the time, but I would give a try to a decorator that catches deprecation and future warnings, ignore them when the version is in range and errors otherwise. I'm sure I did some steps towards that but I cannot find where.

@amueller
Copy link
Member Author

I attempted the thing @massich is mentioning in github.com/amueller/futurepast but didn't have the follow-through necessary

@@ -75,6 +76,7 @@ def test_delegate_to_func():
)


@ignore_warnings(FutureWarning) # ignore warning for validate=False 0.22
Copy link
Contributor

Choose a reason for hiding this comment

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

@ignore_warnings(category=FutureWarning)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants