Skip to content
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] Migrate raising errors from nose to pytest #321

Merged
merged 7 commits into from Aug 24, 2017

Conversation

massich
Copy link
Contributor

@massich massich commented Aug 17, 2017

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
Copy link

codecov bot commented Aug 17, 2017

Codecov Report

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

Impacted file tree graph

@@            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.

@massich massich mentioned this pull request Aug 18, 2017
15 tasks
@pep8speaks
Copy link

pep8speaks commented Aug 18, 2017

Hello @massich! Thanks for updating the PR.

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

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

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

massich commented Aug 18, 2017

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
Copy link
Member

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.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should change to 2.7. cc: @glemaitre

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

you can use +NORMALIZE_WHITESPACE

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@massich massich Aug 22, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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
Copy link
Member

Choose a reason for hiding this comment

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

you can use +NORMALIZE_WHITESPACE

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
Copy link
Member

Choose a reason for hiding this comment

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

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.

@glemaitre glemaitre changed the title [MRG] Migrate raising errors from nose to pytest [WIP] Migrate raising errors from nose to pytest Aug 22, 2017
@massich massich force-pushed the nose_to_pytest branch 4 times, most recently from a46ade0 to 09a744a Compare August 23, 2017 15:57
@massich
Copy link
Contributor Author

massich commented Aug 23, 2017

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

@massich massich changed the title [WIP] Migrate raising errors from nose to pytest [MRG] Migrate raising errors from nose to pytest Aug 23, 2017
...
Failed: DID NOT WARN. No warnings of type (<class 'RuntimeWarning'>,) was emitted. The list of emitted warnings is: [UserWarning(<class 'UserWarning'>,)].


Copy link
Member

Choose a reason for hiding this comment

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

remove two blanks lines

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Change to "Testing utilities"


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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

from sklearn.utils.testing import assert_warns_message
from imblearn.utils.testing import warns

from pytest import raises
Copy link
Member

Choose a reason for hiding this comment

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

this import need to be before the imblearn import

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer under numpy in fact

from sklearn.ensemble import RandomForestClassifier

from imblearn.ensemble import BalanceCascade

from pytest import raises
Copy link
Member

Choose a reason for hiding this comment

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

before imblearn import (the best would be under numpy)

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

move it under numpy

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

move under numpy

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

above the scikit-learn import

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

from sklearn.base import BaseEstimator

from pytest import warns as _warns
from contextlib import contextmanager
from re import compile
Copy link
Member

Choose a reason for hiding this comment

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

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


@contextmanager
def warns(expected_warning, match=None):
"""
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

You can add a small example as well.

from sklearn.utils.validation import check_X_y, check_array

from imblearn.utils.estimator_checks import check_estimator

from pytest import raises
Copy link
Member

Choose a reason for hiding this comment

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

above sklearn

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
Copy link
Member

Choose a reason for hiding this comment

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

above sklearn

@massich
Copy link
Contributor Author

massich commented Aug 24, 2017

ping @glemaitre

@glemaitre
Copy link
Member

@pep8speaks

@glemaitre
Copy link
Member

good to be merge when green

@glemaitre glemaitre merged commit 488a0e8 into scikit-learn-contrib:master Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants