Repository: scikit-learn/scikit-learn
Pull request: [MRG+2] catch more expected warnings in common tests
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#pull-request-checklist
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #11109. See also #7006.

#### What does this implement/fix? Explain your changes.
1. Ignore the expected warning information in common tests, including DeprecationWarning, ConvergenceWarning, UserWarning and FutureWarning. 
2. Removed unnecessary `warnings.resetfilters` in `clean_warning_registry()`.
3. Changed `ignore_warnings` to ignore both local warnings and external warnings like [suppress_warnings](https://github.com/numpy/numpy/blob/00b7d4ab73f8f394b771de2bbab8a3a4acdbb635/numpy/testing/_private/utils.py#L1946-L1948) in numpy.
4. Fixed a bug in using `ignore_warnings` as context manager, restore warning filters upon exiting, avoid affecting non-local filters.

#### Any other comments?


<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
catch more warnings in common tests
Right now the output of travis is too large to render, partially because a lot of deprecation warnings and partially because of warnings from the common tests. The common tests should catch all the deprecation warnings and probably also convergence warnings or numerical warnings (we decrease the number of iterations for faster testing).

Hints from issue discussion:
Do you mean deprecation warnings like the one below? If you can guide me a little, I am happy to work on this issue. 
![image](https://user-images.githubusercontent.com/5948157/40274153-453622a6-5c02-11e8-8e50-29d341612e02.png)

I meant deprecation warnings raised in scikit-learn, not in Numpy. That one you mention is during compilation. I meant during testing.
On your machine, you can run the tests with `pytest -r a sklearn`. At the end of the script, pytest prints a "warnings summary". It would be great to silence the expected warnings, such as convergence warnings.

Example: `pytest -r a sklearn/linear_model/tests/test_coordinate_descent.py`:
```
sklearn/linear_model/tests/test_coordinate_descent.py ...................................

=========================================================== warnings summary ============================================================
sklearn/linear_model/tests/test_coordinate_descent.py::test_lasso_cv_with_some_model_selection
  /cal/homes/tdupre/work/src/scikit-learn/sklearn/model_selection/_split.py:605: Warning: The least populated class in y has only 1 members, which is too few. The minimum number of members in any class cannot be less than n_splits=5.
    % (min_groups, self.n_splits)), Warning)

sklearn/linear_model/tests/test_coordinate_descent.py::test_multitask_enet_and_lasso_cv
  /cal/homes/tdupre/work/src/scikit-learn/sklearn/linear_model/coordinate_descent.py:1783: ConvergenceWarning: Objective did not converge, you might want to increase the number of iterations
    ConvergenceWarning)
  /cal/homes/tdupre/work/src/scikit-learn/sklearn/linear_model/coordinate_descent.py:1783: ConvergenceWarning: Objective did not converge, you might want to increase the number of iterations
    ConvergenceWarning)

sklearn/linear_model/tests/test_coordinate_descent.py::test_check_input_false
  /cal/homes/tdupre/work/src/scikit-learn/sklearn/linear_model/coordinate_descent.py:491: ConvergenceWarning: Objective did not converge. You might want to increase the number of iterations. Fitting data with very small alpha may cause precision problems.
    ConvergenceWarning)
```

To silence warnings, see `sklearn.utils.testing.ignore_warnings`:
https://github.com/scikit-learn/scikit-learn/blob/4b24fbedf5fa3b7b6b559141ad78708145b09704/sklearn/utils/testing.py#L269-L292
Hi, I am looking into this issue, I figure the "common tests" means the test_common.py under the tests directory? I tried running the test but no warning is generated, could you tell me what warning is expected? Thanks a lot.
```
============================================= test session starts =============================================
platform darwin -- Python 3.6.1, pytest-3.0.7, py-1.4.33, pluggy-0.4.0
rootdir: /Users/sw/programming/opensourceproject/scikit-learn, inifile: setup.cfg
plugins: pep8-1.0.6
collected 4822 items

test_common.py .........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................s............................................................................s............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................s.........................................................................................................................................................................................................................................................................................................s................................................................................................................................................................................................................................s..................................s..............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................s............................................................................................................................................................................................................................................................................s........................................................................................................................................................................................................................................................................................................................................................................................................................
=========================================== short test summary info ===========================================
SKIP [1] /anaconda/lib/python3.6/site-packages/_pytest/nose.py:23: score_samples of BernoulliRBM is not invariant when applied to a subset.
SKIP [3] /anaconda/lib/python3.6/site-packages/_pytest/nose.py:23: Skipping check_estimators_data_not_an_array for cross decomposition module as estimators are not deterministic.
SKIP [1] /anaconda/lib/python3.6/site-packages/_pytest/nose.py:23: transform of MiniBatchSparsePCA is not invariant when applied to a subset.
SKIP [1] /anaconda/lib/python3.6/site-packages/_pytest/nose.py:23: Not testing NuSVC class weight as it is ignored.
SKIP [1] /anaconda/lib/python3.6/site-packages/_pytest/nose.py:23: decision_function of SVC is not invariant when applied to a subset.
SKIP [1] /anaconda/lib/python3.6/site-packages/_pytest/nose.py:23: transform of SparsePCA is not invariant when applied to a subset.

=================================== 4814 passed, 8 skipped in 72.05 seconds ===================================
```

>  figure the "common tests" means the test_common.py under the tests directory? 

Yes

> I tried running the test but no warning is generated

You need to add the `-r a` pytest CLI option as indicated in https://github.com/scikit-learn/scikit-learn/issues/11109#issuecomment-391061304
Huh, That's weird. I did have `-r a`, and the command was: `pytest -r a sklearn/tests/test_common.py`. But as shown above, no warning was generated, I tried `pytest -r a sklearn/tests/test_common.py --strict` and found one deprecationwarning and a bunch of errors.
figured out, I was using an older pytest verision.
Last time I looked at this, I realised that one of the problem (there may be more) was that `assert_warns_*` resets `warnings.filters` which overrides the `ignore_warnings` used as a decorator. It does not seem like a great idea and the reason for it is slightly unclear. Below is a snippet to show the problem:

```py
import warnings

import pytest

from sklearn.utils.testing import ignore_warnings
from sklearn.utils.testing import assert_warns_message


def warns():
    warnings.warn("some warning")
    return 1


@ignore_warnings()
def test_1():
    print('before:', warnings.filters)
    assert_warns_message(UserWarning, 'some warning', warns)
    print('after:', warnings.filters)
    # This warning is visible because assert_warns_message resets
    # warnings.filters.
    warns()


def test_12():
    print('test12:', warnings.filters)


ignore_common_warnings = pytest.mark.filterwarnings('ignore::UserWarning')


@ignore_common_warnings
def test_2():
    warns()


@ignore_common_warnings
def test_3():
    assert_warns_message(UserWarning, 'some warning', warns)
    # This warning is visible
    warns()

```

```
❯ pytest /tmp/test-warnings.py -s 
======================================================== test session starts ========================================================
platform linux -- Python 3.6.5, pytest-3.5.1, py-1.5.3, pluggy-0.6.0
rootdir: /tmp, inifile:
plugins: timeout-1.2.1, flake8-0.9.1, cov-2.5.1, hypothesis-3.56.0
collected 3 items                                                                                                                   

../../../../../tmp/test-warnings.py before: [('ignore', None, <class 'Warning'>, None, 0)]
after: []
...

========================================================= warnings summary ==========================================================
test-warnings.py::test_1
  /tmp/test-warnings.py:10: UserWarning: some warning
    warnings.warn("some warning")

test-warnings.py::test_3
  /tmp/test-warnings.py:10: UserWarning: some warning
    warnings.warn("some warning")

-- Docs: http://doc.pytest.org/en/latest/warnings.html
=============================================== 3 passed, 2 warnings in 0.18 seconds ================================================
```
Interesting. I would be in favor of replacing `assert_warns_message(UserWarning, 'some warning', warn)` with,
```py
with pytest.warns(UserWarning, match='some warning'):
   warn()
```
to avoid dealing with it..
If someone has a semi-automated way of replacing the `assert_warns*`, why not, but it could well be the case that some downstream packages are using them.

Maybe implementing `assert_warns_*` in terms of `pytest_warns` is a less tedious and less intrusive change to implement, i.e. something like this (not tested):

```py
def assert_warns_regex(warning_cls, warning_message, func, *args, **kwargs):
    with pytest.warns(warning_cls, match=re.escape(warning_message)):
        func(*args, **kwargs)
```

Commit message: remove reset warning in assert_warns

Patch to review:
diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py
index 62a3bee5fc12d..1ec855fe67a37 100644
--- a/sklearn/tests/test_common.py
+++ b/sklearn/tests/test_common.py
@@ -22,6 +22,7 @@
 from sklearn.utils.testing import assert_greater
 from sklearn.utils.testing import assert_in
 from sklearn.utils.testing import ignore_warnings
+from sklearn.exceptions import ConvergenceWarning
 
 import sklearn
 from sklearn.cluster.bicluster import BiclusterMixin
@@ -90,19 +91,23 @@ def _rename_partial(val):
         ids=_rename_partial
 )
 def test_non_meta_estimators(name, Estimator, check):
-    # Common tests for non-meta estimators
-    estimator = Estimator()
-    set_checking_parameters(estimator)
-    check(name, estimator)
+    with ignore_warnings(category=(DeprecationWarning, ConvergenceWarning,
+                                   UserWarning, FutureWarning)):
+        # Common tests for non-meta estimators
+        estimator = Estimator()
+        set_checking_parameters(estimator)
+        check(name, estimator)
 
 
 @pytest.mark.parametrize("name, Estimator",
                          _tested_non_meta_estimators())
 def test_no_attributes_set_in_init(name, Estimator):
-    # input validation etc for non-meta estimators
-    estimator = Estimator()
-    # check this on class
-    check_no_attributes_set_in_init(name, estimator)
+    with ignore_warnings(category=(DeprecationWarning, ConvergenceWarning,
+                                   UserWarning, FutureWarning)):
+        # input validation etc for non-meta estimators
+        estimator = Estimator()
+        # check this on class
+        check_no_attributes_set_in_init(name, estimator)
 
 
 def test_configure():
diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py
index 5e4c454f4b1ab..80acbfc015e56 100644
--- a/sklearn/utils/estimator_checks.py
+++ b/sklearn/utils/estimator_checks.py
@@ -301,10 +301,10 @@ def check_estimator(Estimator):
     for check in _yield_all_checks(name, estimator):
         try:
             check(name, estimator)
-        except SkipTest as message:
+        except SkipTest as exception:
             # the only SkipTest thrown currently results from not
             # being able to import pandas.
-            warnings.warn(message, SkipTestWarning)
+            warnings.warn(str(exception), SkipTestWarning)
 
 
 def _boston_subset(n_samples=200):
diff --git a/sklearn/utils/testing.py b/sklearn/utils/testing.py
index 6f959687c56ed..be94a2cb3fd22 100644
--- a/sklearn/utils/testing.py
+++ b/sklearn/utils/testing.py
@@ -339,13 +339,13 @@ def __repr__(self):
 
     def __enter__(self):
         clean_warning_registry()  # be safe and not propagate state + chaos
-        warnings.simplefilter("ignore", self.category)
         if self._entered:
             raise RuntimeError("Cannot enter %r twice" % self)
         self._entered = True
         self._filters = self._module.filters
         self._module.filters = self._filters[:]
         self._showwarning = self._module.showwarning
+        warnings.simplefilter("ignore", self.category)
 
     def __exit__(self, *exc_info):
         if not self._entered:
@@ -729,8 +729,6 @@ def run_test(*args, **kwargs):
 
 
 def clean_warning_registry():
-    """Safe way to reset warnings."""
-    warnings.resetwarnings()
     reg = "__warningregistry__"
     for mod_name, mod in list(sys.modules.items()):
         if 'six.moves' in mod_name:
diff --git a/sklearn/utils/tests/test_testing.py b/sklearn/utils/tests/test_testing.py
index 6b55431d21d7d..919c7205f00b0 100644
--- a/sklearn/utils/tests/test_testing.py
+++ b/sklearn/utils/tests/test_testing.py
@@ -211,26 +211,20 @@ def context_manager_no_user_multiple_warning():
     assert_warns(DeprecationWarning, context_manager_no_user_multiple_warning)
 
 
-# This class is inspired from numpy 1.7 with an alteration to check
-# the reset warning filters after calls to assert_warns.
-# This assert_warns behavior is specific to scikit-learn because
-# `clean_warning_registry()` is called internally by assert_warns
-# and clears all previous filters.
+# assert_warns will no longer clear all previous filters.
 class TestWarns(unittest.TestCase):
     def test_warn(self):
         def f():
             warnings.warn("yo")
             return 3
 
-        # Test that assert_warns is not impacted by externally set
-        # filters and is reset internally.
-        # This is because `clean_warning_registry()` is called internally by
-        # assert_warns and clears all previous filters.
+        # test that assert_warns doesn't affect external filters
+        warnings.resetwarnings()
         warnings.simplefilter("ignore", UserWarning)
         assert_equal(assert_warns(UserWarning, f), 3)
 
-        # Test that the warning registry is empty after assert_warns
-        assert_equal(sys.modules['warnings'].filters, [])
+        assert_equal(warnings.filters,
+                     [('ignore', None, UserWarning, None, 0)])
 
         assert_raises(AssertionError, assert_no_warnings, f)
         assert_equal(assert_no_warnings(lambda x: x, 1), 1)


Code review comments:
diff_hunk: @@ -211,26 +211,20 @@ def context_manager_no_user_multiple_warning():
     assert_warns(DeprecationWarning, context_manager_no_user_multiple_warning)
 
 
-# This class is inspired from numpy 1.7 with an alteration to check
-# the reset warning filters after calls to assert_warns.
-# This assert_warns behavior is specific to scikit-learn because
-# `clean_warning_registry()` is called internally by assert_warns
-# and clears all previous filters.
+# assert_warns will no longer clear all previous filters.
 class TestWarns(unittest.TestCase):
     def test_warn(self):
         def f():
             warnings.warn("yo")
             return 3
 
-        # Test that assert_warns is not impacted by externally set
-        # filters and is reset internally.
-        # This is because `clean_warning_registry()` is called internally by
-        # assert_warns and clears all previous filters.
+        # test that assert_warns doesn't affect external filters
+        warnings.resetwarnings()
path: sklearn/utils/tests/test_testing.py
text: @user1:
Please do not mess with the global `warnings.filters`. Use a `warnings.catch_warnings()` context manager if you want to use `warwnings.resetwarnings`, `warning.simplefilter` or `warnings.filterwarnings`.

Basically do:
```py
with warnings.catch_warnings():
    warnings.simplefilter('ignore', UserWarning)
    filters_orig = warnings.filters[:]
    assert_equal(assert_warns(UserWarning, f), 3)
    assert_equal(warnings.filters, filters_orig)
```

I added something that checks that the warnings.filters are not affected by assert_warns (not tested)

diff_hunk: @@ -211,26 +211,20 @@ def context_manager_no_user_multiple_warning():
     assert_warns(DeprecationWarning, context_manager_no_user_multiple_warning)
 
 
-# This class is inspired from numpy 1.7 with an alteration to check
-# the reset warning filters after calls to assert_warns.
-# This assert_warns behavior is specific to scikit-learn because
-# `clean_warning_registry()` is called internally by assert_warns
-# and clears all previous filters.
+# assert_warns will no longer clear all previous filters.
path: sklearn/utils/tests/test_testing.py
text: @user1:
Please remove this comment, it does add much value.

@author:
Didn't quite understand, do you mean it is not true?