[MRG] Migrate raising errors from nose to pytest #321

massich · 2017-08-17T18:04:34Z

Reference Issue

Partial migration from nose to pytest (see #323)

What does this implement/fix? Explain your changes.

Things related to raising. The idea is to migrate or change the following:

from sklearn.utils.testing import assert_raises
from sklearn.utils.testing import assert_raises_regex
from sklearn.utils.testing import assert_raise_message
from sklearn.utils.testing import assert_warns
from sklearn.utils.testing import assert_warns_message

* [ ] from sklearn.utils.testing import assert_no_warnings

Without touching any testing logic.

Any other comments?

Do not squash this PR (I've reworked the git history for easier inspection). And I would recommend to review each commit of the PR independently.

codecov · 2017-08-17T19:20:20Z

Codecov Report

Merging #321 into master will increase coverage by 0.18%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #321      +/-   ##
==========================================
+ Coverage      98%   98.19%   +0.18%     
==========================================
  Files          66       66              
  Lines        3860     3980     +120     
==========================================
+ Hits         3783     3908     +125     
+ Misses         77       72       -5

Impacted Files	Coverage Δ
imblearn/ensemble/tests/test_balance_cascade.py	`100% <100%> (ø)`	⬆️
..._sampling/prototype_selection/tests/test_allknn.py	`100% <100%> (ø)`	⬆️
imblearn/utils/testing.py	`100% <100%> (ø)`	⬆️
..._selection/tests/test_edited_nearest_neighbours.py	`100% <100%> (ø)`	⬆️
imblearn/datasets/tests/test_imbalance.py	`100% <100%> (ø)`	⬆️
imblearn/combine/tests/test_smote_enn.py	`100% <100%> (ø)`	⬆️
imblearn/tests/test_exceptions.py	`100% <100%> (ø)`	⬆️
imblearn/combine/tests/test_smote_tomek.py	`100% <100%> (ø)`	⬆️
imblearn/utils/estimator_checks.py	`92.76% <100%> (+3.15%)`	⬆️
...election/tests/test_instance_hardness_threshold.py	`100% <100%> (ø)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2de53a...4be2547. Read the comment docs.

pep8speaks · 2017-08-18T13:36:20Z

Hello @massich! Thanks for updating the PR.

In the file imblearn/utils/tests/test_estimator_checks.py, following are the PEP8 issues :

Line 149:5: E722 do not use bare except'

Comment last updated on August 24, 2017 at 15:40 Hours UTC

massich · 2017-08-18T14:01:44Z

Regarding from sklearn.utils.testing import assert_raise_message, shall keep using message or we can see it like a particular case of regex were everything matches? And as a related thought, shall we move the tests that use regex with the whole error message and use only particularities?

cc: @glemaitre, @chkoar

glemaitre · 2017-08-18T14:03:08Z

Regarding from sklearn.utils.testing import assert_raise_message, shall keep using message or we can see it like a particular case of regex were everything matches? And as a related thought, shall we move the tests that use regex with the whole error message and use only particularities?

move to regex. for the second thing, only the semantic is necessary. that's why I like regex.

massich · 2017-08-21T16:28:49Z

imblearn/utils/testing.py

+    Assert that a code block/function call warns ``expected_warning``
+    and raise a failure exception otherwise.
+
+    If using Python 2.5 or above, you may use this function as a


This should change to 2.7. cc: @glemaitre

and the docstring lines should be truncated to 80 characters. But I don't know if this would kill the doctest. Does anyone know it?

you can use +NORMALIZE_WHITESPACE

you need also to use a more numpydoc way using Parameters Returns and Examples.
I would add a narrative documentation in the user guide instead of the docstring.

I think that the structure of this narrative documentation should be done in a different PR (merge it before this one) and add the relevant information regarding warns in this PR.

edit: I've created #325 for such purpose

you can make a PR adding warns + documentation if you which

glemaitre · 2017-08-21T16:36:07Z

imblearn/utils/testing.py

+    Assert that a code block/function call warns ``expected_warning``
+    and raise a failure exception otherwise.
+
+    If using Python 2.5 or above, you may use this function as a


you can use +NORMALIZE_WHITESPACE

glemaitre · 2017-08-21T16:37:06Z

imblearn/utils/testing.py

+    Assert that a code block/function call warns ``expected_warning``
+    and raise a failure exception otherwise.
+
+    If using Python 2.5 or above, you may use this function as a


you need also to use a more numpydoc way using Parameters Returns and Examples.
I would add a narrative documentation in the user guide instead of the docstring.

massich · 2017-08-23T17:30:06Z

I think I'll finish this PR here and ask for reviews (cc:@glemaitre, @chkoar, @mrastgoo).
And move the deprecation warnings and assert_no_warngings in different PRs.

Refer to this for assert_no_warnings

glemaitre · 2017-08-23T17:48:59Z

doc/developers_utils.rst

+      ...
+    Failed: DID NOT WARN. No warnings of type (<class 'RuntimeWarning'>,) was emitted. The list of emitted warnings is: [UserWarning(<class 'UserWarning'>,)].
+
+


remove two blanks lines

glemaitre · 2017-08-23T17:50:18Z

doc/developers_utils.rst

@@ -96,3 +96,45 @@ same information as the deprecation warning as explained above. Use the
 On the top of all the functionality provided by scikit-learn. Imbalance-learn
 provides :func:`deprecate_parameter`: which is used to deprecate a sampler's
 parameter (attribute) by another one.
+
+Warning management when testing


Change to "Testing utilities"

glemaitre · 2017-08-23T17:52:25Z

doc/developers_utils.rst

+
+Warning management when testing
+===============================
+


Currently, imbalanced-learn provide a warning management utility. This feature is going to be merge in pytest and will be removed when the pytest release will have it.

glemaitre · 2017-08-24T09:08:20Z

You should check my previous comment. Since that you pushed they get folded and outdated but they need to be addressed.

glemaitre

Apart of the import ordered and the doc of warns it LGTM

glemaitre · 2017-08-24T09:52:06Z

doc/developers_utils.rst

+    ...    pass
+    Traceback (most recent call last):
+      ...
+    Failed: DID NOT WARN. No warnings of type (<.*RuntimeWarning.*>,) was emitted. The list of emitted warnings is: [].


I did not think that doctest is an exact matching. You probably have to check in the doctest doc how to make regular expression or use the ellipsis as work around

glemaitre · 2017-08-24T09:53:03Z

imblearn/datasets/tests/test_imbalance.py

-from sklearn.utils.testing import assert_warns_message
+from imblearn.utils.testing import warns
+
+from pytest import raises


this import need to be before the imblearn import

I would prefer under numpy in fact

glemaitre · 2017-08-24T09:54:02Z

imblearn/ensemble/tests/test_balance_cascade.py

 from sklearn.ensemble import RandomForestClassifier

 from imblearn.ensemble import BalanceCascade

+from pytest import raises


before imblearn import (the best would be under numpy)

glemaitre · 2017-08-24T09:54:31Z

imblearn/combine/tests/test_smote_tomek.py

@@ -7,7 +7,7 @@

 import numpy as np
 from sklearn.utils.testing import assert_allclose, assert_array_equal
-from sklearn.utils.testing import assert_raises_regex
+from pytest import raises


move it under numpy

glemaitre · 2017-08-24T09:54:38Z

imblearn/combine/tests/test_smote_enn.py

@@ -7,7 +7,7 @@

 import numpy as np
 from sklearn.utils.testing import assert_allclose, assert_array_equal
-from sklearn.utils.testing import assert_raises_regex
+from pytest import raises


move under numpy

glemaitre · 2017-08-24T10:00:37Z

imblearn/utils/estimator_checks.py

@@ -18,7 +18,9 @@
    as sklearn_yield_all_checks, check_estimator \
    as sklearn_check_estimator, check_parameters_default_constructible
 from sklearn.exceptions import NotFittedError
-from sklearn.utils.testing import assert_warns, assert_raises_regex
+from pytest import raises


above the scikit-learn import

glemaitre · 2017-08-24T10:02:04Z

imblearn/utils/testing.py

@@ -14,6 +14,11 @@

 from sklearn.base import BaseEstimator

+from pytest import warns as _warns
+from contextlib import contextmanager
+from re import compile


the order will be:

standard library

3rd party library

own library

then

move context and re on the top
then pytest
then scikit-learn
then imblearn

glemaitre · 2017-08-24T10:08:05Z

imblearn/utils/testing.py

+
+@contextmanager
+def warns(expected_warning, match=None):
+    """


you need to improve the docstring

def warns(expected_warning, match=None): """Assert a warning is raised with an optional pattern. Assert that a code block/function call warns ``expected_warning`` and raise a failure exception otherwise. It can be used within a context manager ``with``. Parameters ---------- expected_warning : Warning Warning type. match : regex str or None, optional The pattern to be checked. By default, no check is done. Returns ------- None """

Check the english because it is on the top of the head

You can add a small example as well.

glemaitre · 2017-08-24T10:08:52Z

imblearn/utils/tests/test_estimator_checks.py

 from sklearn.utils.validation import check_X_y, check_array

 from imblearn.utils.estimator_checks import check_estimator

+from pytest import raises


above sklearn

glemaitre · 2017-08-24T10:09:23Z

imblearn/utils/tests/test_validation.py

-from sklearn.utils.testing import assert_raises_regex
-from sklearn.utils.testing import assert_warns_message
+from imblearn.utils.testing import warns
+from pytest import raises


above sklearn

massich · 2017-08-24T15:40:38Z

ping @glemaitre

glemaitre · 2017-08-24T15:54:12Z

@pep8speaks

glemaitre · 2017-08-24T15:55:25Z

good to be merge when green

massich mentioned this pull request Aug 18, 2017

pytest follow up #323

Closed

15 tasks

massich force-pushed the nose_to_pytest branch from deec149 to c995c8b Compare August 18, 2017 13:36

massich force-pushed the nose_to_pytest branch from c995c8b to 8d2a1dd Compare August 18, 2017 13:52

massich changed the title ~~[MRG] change assert_raise for raises(xxx)~~ [MRG] Migrate raising errors from nose to pytest Aug 18, 2017

massich mentioned this pull request Aug 21, 2017

[WIP] Add testing.py and tests pydicom/pydicom#488

Closed

massich commented Aug 21, 2017

View reviewed changes

glemaitre approved these changes Aug 21, 2017

View reviewed changes

massich force-pushed the nose_to_pytest branch from 6014b7e to 8da92de Compare August 21, 2017 16:47

glemaitre changed the title ~~[MRG] Migrate raising errors from nose to pytest~~ [WIP] Migrate raising errors from nose to pytest Aug 22, 2017

glemaitre mentioned this pull request Aug 22, 2017

[MRG] EHN handling sparse matrices whenever possible #316

Merged

26 tasks

massich force-pushed the nose_to_pytest branch 4 times, most recently from a46ade0 to 09a744a Compare August 23, 2017 15:57

massich force-pushed the nose_to_pytest branch from 09a744a to e161fa1 Compare August 23, 2017 17:46

Joan Massich added 3 commits August 23, 2017 19:48

change assert_raise for raises(xxx)

f0d70c0

Migrate assert_raises_regex to pytest.raises

74cefc2

Change assert_raise_message to raises with regexp

a60399a

massich force-pushed the nose_to_pytest branch from e161fa1 to 66f7ea4 Compare August 23, 2017 17:51

massich changed the title ~~[WIP] Migrate raising errors from nose to pytest~~ [MRG] Migrate raising errors from nose to pytest Aug 23, 2017

glemaitre reviewed Aug 23, 2017

View reviewed changes

massich force-pushed the nose_to_pytest branch from 66f7ea4 to d206aa4 Compare August 23, 2017 17:58

glemaitre reviewed Aug 24, 2017

View reviewed changes

massich force-pushed the nose_to_pytest branch from d206aa4 to a127230 Compare August 24, 2017 13:57

massich force-pushed the nose_to_pytest branch from a127230 to 2077fca Compare August 24, 2017 14:47

Joan Massich added 3 commits August 24, 2017 17:16

add warns context manger that mimics raises

07e99e1

Move assert_warns to imblearn.utils.test.warns

572084f

migrate assert_warns_message to imblearn.utils.testing.warns

dd2b0db

massich force-pushed the nose_to_pytest branch from 47ea290 to dd2b0db Compare August 24, 2017 15:16

Move import statements for codebase coherence.

4be2547

massich force-pushed the nose_to_pytest branch from 1acfd30 to 4be2547 Compare August 24, 2017 15:40

glemaitre merged commit 488a0e8 into scikit-learn-contrib:master Aug 24, 2017

		...
		Failed: DID NOT WARN. No warnings of type (<class 'RuntimeWarning'>,) was emitted. The list of emitted warnings is: [UserWarning(<class 'UserWarning'>,)].


		Warning management when testing
		===============================

[MRG] Migrate raising errors from nose to pytest #321

[MRG] Migrate raising errors from nose to pytest #321

Uh oh!

Conversation

massich commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

codecov bot commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pep8speaks commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on August 24, 2017 at 15:40 Hours UTC

Uh oh!

massich commented Aug 18, 2017

Uh oh!

glemaitre commented Aug 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

massich Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

massich commented Aug 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Aug 24, 2017

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

massich commented Aug 24, 2017

Uh oh!

glemaitre commented Aug 24, 2017

massich commented Aug 17, 2017 •

edited

Loading

codecov bot commented Aug 17, 2017 •

edited

Loading

pep8speaks commented Aug 18, 2017 •

edited

Loading

massich Aug 22, 2017 •

edited

Loading