[MRG] nosetests to pytests #310

massich · 2017-08-02T16:20:10Z

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

pep8speaks · 2017-08-02T16:20:17Z

Hello @massich! Thanks for updating the PR.

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

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

Comment last updated on August 17, 2017 at 15:57 Hours UTC

codecov · 2017-08-02T16:28:01Z

Codecov Report

Merging #310 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #310      +/-   ##
========================================
+ Coverage   97.99%    98%   +<.01%     
========================================
  Files          66     66              
  Lines        3847   3860      +13     
========================================
+ Hits         3770   3783      +13     
  Misses         77     77

Impacted Files	Coverage Δ
imblearn/over_sampling/tests/test_smote.py	`100% <100%> (ø)`	⬆️
imblearn/utils/tests/test_estimator_checks.py	`91.95% <100%> (ø)`	⬆️
imblearn/combine/tests/test_smote_enn.py	`100% <100%> (ø)`	⬆️
...election/tests/test_instance_hardness_threshold.py	`100% <100%> (ø)`	⬆️
...n/tests/test_repeated_edited_nearest_neighbours.py	`100% <100%> (ø)`	⬆️
imblearn/combine/tests/test_smote_tomek.py	`100% <100%> (ø)`	⬆️
imblearn/utils/estimator_checks.py	`89.61% <100%> (+0.06%)`	⬆️
imblearn/over_sampling/tests/test_adasyn.py	`100% <100%> (ø)`	⬆️
imblearn/tests/test_common.py	`94.73% <100%> (-0.51%)`	⬇️
..._selection/tests/test_edited_nearest_neighbours.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 23dc2b7...dfda9f6. Read the comment docs.

chkoar · 2017-08-07T07:35:39Z

@massich thanks for this one!

A little note. Instead of creating objects couldn't we use pytest fixtures?
For instance lets see the test module for ROS.

We could create a sampler object

@pytest.fixture
def sampler():
    return RandomOverSampler(random_state=RND_SEED)

and then pass it in any test case

def test_ros_fit_sample_half(sampler):
    ratio = 0.5
    X_resampled, y_resampled = sampler.fit_sample(X, Y)
    X_gt = np.array([[0.04352327, -0.20515826],
                     [0.92923648, 0.76103773],
                     [0.20792588, 1.49407907],
                     [0.47104475, 0.44386323],
                     [0.22950086, 0.33367433],
                     [0.15490546, 0.3130677],
                     [0.09125309, -0.85409574],
                     [0.12372842, 0.6536186],
                     [0.13347175, 0.12167502],
                     [0.094035, -2.55298982]])
    y_gt = np.array([1, 0, 1, 0, 1, 1, 1, 1, 0, 1])
    assert_array_equal(X_resampled, X_gt)
    assert_array_equal(y_resampled, y_gt)

Of course, the same could have been applied for the datasets as well.

massich · 2017-08-07T08:16:56Z

@chkoar This was exactly the point :) (its just that I'm not quite done. I'm the slow kind)

massich · 2017-08-17T14:30:05Z

Right now only assert_equal and similar things are migrated. For completeness, here are two reference regarding:
numpy equality and almost equal

glemaitre

I just have to check RENN n_jobs

glemaitre · 2017-08-17T14:01:38Z

imblearn/ensemble/tests/test_easy_ensemble.py

-    assert_equal(ee.n_subsets, 10)
-    assert_equal(ee.random_state, RND_SEED)
+    assert ee.ratio == ratio
+    assert not ee.replacement


assert ee.replacement is False

glemaitre · 2017-08-17T14:05:58Z

imblearn/metrics/tests/test_classification.py

@@ -436,22 +428,22 @@ def test_iba_sklearn_metrics():
    acc = make_index_balanced_accuracy(alpha=0.5, squared=True)(
        accuracy_score)
    score = acc(y_true, y_pred)
-    assert_equal(score, 0.54756)
+    assert score == 0.54756


can you replace those by assert_almost_equal instead.

or assert_allclose because we use it before in fact

the numpy doc recommend the latests one

glemaitre · 2017-08-17T14:06:02Z

imblearn/metrics/tests/test_classification.py


    jss = make_index_balanced_accuracy(alpha=0.5, squared=True)(
        jaccard_similarity_score)
    score = jss(y_true, y_pred)
-    assert_equal(score, 0.54756)
+    assert score == 0.54756


can you replace those by assert_almost_equal instead.

assert_allclose

shouldn't it be better to do:

from pytest import approx assert score == approx(0.54756)

glemaitre · 2017-08-17T14:06:07Z

imblearn/metrics/tests/test_classification.py


    pre = make_index_balanced_accuracy(alpha=0.5, squared=True)(
        precision_score)
    score = pre(y_true, y_pred)
-    assert_equal(score, 0.65025)
+    assert score == 0.65025


can you replace those by assert_almost_equal instead.

assert_allclose

glemaitre · 2017-08-17T14:06:12Z

imblearn/metrics/tests/test_classification.py


    rec = make_index_balanced_accuracy(alpha=0.5, squared=True)(
        recall_score)
    score = rec(y_true, y_pred)
-    assert_equal(score, 0.41616000000000009)
+    assert score == 0.41616000000000009


can you replace those by assert_almost_equal instead.

assert_allclose

glemaitre · 2017-08-17T14:31:02Z

imblearn/tests/test_pipeline.py

-                       'm2__mult': 2,
-                       'last__mult': 5,
-                       })
+    _expected_params = {'steps': pipeline.steps,


expected_params

glemaitre · 2017-08-17T14:32:09Z

imblearn/tests/test_pipeline.py

@@ -641,7 +636,7 @@ def test_pipeline_memory_transformer():
        cached_pipe.fit(X, y)
        pipe.fit(X, y)
        # Get the time stamp of the tranformer in the cached pipeline
-        ts = cached_pipe.named_steps['transf'].timestamp_
+        _expected_ts = cached_pipe.named_steps['transf'].timestamp_


expected_ts

glemaitre · 2017-08-17T14:33:34Z

imblearn/under_sampling/prototype_selection/tests/test_repeated_edited_nearest_neighbours.py

-    assert_equal(renn.random_state, RND_SEED)
+    assert renn.n_neighbors == 3
+    assert renn.kind_sel == 'all'
+    assert renn.n_jobs == -1


Uhm this is strange we setup n_jobs to 1 normally. there is something fishy

glemaitre · 2017-08-17T14:34:40Z

imblearn/utils/estimator_checks.py

    sampler = Sampler(random_state=0)
+    _expected_stat = Counter(y)[1]


expected_stat

glemaitre · 2017-08-17T14:35:21Z

imblearn/utils/tests/test_validation.py

@@ -46,17 +46,16 @@ def test_check_ratio_error():


 def test_ratio_all_over_sampling():
+    _expected_ratio = {1: 50, 2: 0, 3: 75}


expected_ratio

actually you don't need to define it anymore

I would keep it, for readability. Otherwise I would do something like:

def test_ratio_all_over_sampling(): y = np.array([1] * 50 + [2] * 100 + [3] * 25) for each in ('all', 'auto'): out = check_ratio(each, y, 'over-sampling') assert out == {1: 50, 2: 0, 3: 75}

Not convince since we don't do anything with check_ractio neither with the expected values.

We wont agree on this one. But since its less than 80 characters. I've no more arguments, just preferences.

assert check_ratio(each, y, 'over-sampling') == {1: 50, 2: 0, 3: 75}

glemaitre changed the title ~~nosetests to pytests~~ [WIP] nosetests to pytests Aug 2, 2017

glemaitre force-pushed the master branch 3 times, most recently from 1b22868 to 33660d4 Compare August 11, 2017 14:43

massich force-pushed the pytest_migration branch 8 times, most recently from a9dd328 to d67c301 Compare August 17, 2017 14:16

glemaitre reviewed Aug 17, 2017

View reviewed changes

massich force-pushed the pytest_migration branch 2 times, most recently from dbb2d1e to 78ce012 Compare August 17, 2017 15:23

Apply nose2pytest @pep8speaks

b0dd825

massich force-pushed the pytest_migration branch from 78ce012 to b0dd825 Compare August 17, 2017 15:30

Merge branch 'master' into xx

dfda9f6

massich changed the title ~~[WIP] nosetests to pytests~~ [MRG] nosetests to pytests Aug 17, 2017

glemaitre merged commit 4ea3bcc into scikit-learn-contrib:master Aug 17, 2017

massich deleted the pytest_migration branch August 17, 2017 16:50

massich mentioned this pull request Aug 18, 2017

pytest follow up #323

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] nosetests to pytests #310

[MRG] nosetests to pytests #310

massich commented Aug 2, 2017

pep8speaks commented Aug 2, 2017 •

edited

Loading

codecov bot commented Aug 2, 2017 •

edited

Loading

chkoar commented Aug 7, 2017

massich commented Aug 7, 2017

massich commented Aug 17, 2017

glemaitre left a comment

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

massich Aug 17, 2017 •

edited

Loading

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

glemaitre Aug 17, 2017

massich Aug 17, 2017

glemaitre Aug 17, 2017

massich Aug 17, 2017

		sampler = Sampler(random_state=0)
		_expected_stat = Counter(y)[1]

		@@ -46,17 +46,16 @@ def test_check_ratio_error():


		def test_ratio_all_over_sampling():
		_expected_ratio = {1: 50, 2: 0, 3: 75}

[MRG] nosetests to pytests #310

[MRG] nosetests to pytests #310

Conversation

massich commented Aug 2, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

pep8speaks commented Aug 2, 2017 • edited Loading

Comment last updated on August 17, 2017 at 15:57 Hours UTC

codecov bot commented Aug 2, 2017 • edited Loading

Codecov Report

chkoar commented Aug 7, 2017

massich commented Aug 7, 2017

massich commented Aug 17, 2017

glemaitre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

massich Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Aug 2, 2017 •

edited

Loading

codecov bot commented Aug 2, 2017 •

edited

Loading

massich Aug 17, 2017 •

edited

Loading