[MRG]Replaced all occurrences of assert_true() and assert_false() with assert. #12588

jaskola8 · 2018-11-14T17:41:56Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Replaced all occurrences of assert_true() and assert_false() with assert, kept definitions of replaced functions in sklearn.utils.testing.

Any other comments?

Tested with pytest, flake8 and by using make.

…ert and assert not, kept definitions of replaced functions. Tested with pytest, flake8 and by using make.

jnothman · 2018-11-14T22:37:44Z

Flake8 is not happy

rth

Thanks for working on this!

A few comments are below. Generally a few remarks (I have not commented in all the places where these apply),

in assert not(result), parentheses are not needed, use assert not result.
For line breaks in assert, see https://stackoverflow.com/a/16065512/1791279
I find it OK (for lack of better alternatives) to use line break for a single line, e.g.
```
assert result, \
     "some long error message"
```
however, when you get more than one line, use parentheses,
```
   assert result, (
     "some long error message: {}"
     "another line" 
     .format(custom_message))
```

sklearn/cluster/tests/test_affinity_propagation.py

sklearn/feature_extraction/tests/test_text.py

sklearn/model_selection/tests/test_validation.py

sklearn/utils/mocking.py

sklearn/utils/tests/test_multiclass.py

jaskola8 · 2018-11-15T12:22:12Z

Thanks for feedback! I'll clean this up today.

qinhanmin2014 · 2018-11-20T03:31:44Z

@jaskola8 any updates?

…embership cleanup) for first half of tests (including models_selection/tests/test_validation.py).

qinhanmin2014 · 2018-11-27T02:57:39Z

@jaskola8 Why WIP? You don't need to worry about the first job of Travis now. ping when it's ready for a second review.

jaskola8 · 2018-11-27T18:30:06Z

I wasn't sure how to indicate that this branch is not ready for merge so i changed [MRG] to [WIP]. If integration tests running now will be successful i will change it to [MRG].

rth

Thank you for this work @jaskola8 !

A few minor comments below, but apart from that this LGTM.

rth · 2018-11-27T19:48:43Z

sklearn/model_selection/tests/test_validation.py

+                'MockClassifier extra fit_param ' 
+                'sample_weight.shape[0]' 
+                ' is {0}, should be {1}'.format(sample_weight.shape[0],
+                                                X.shape[0]))


Nit (only if you do changes for other reviewers), this could be more nicely written as,

'sample_weight.shape[0] is {0}, should be {1}' .format(sample_weight.shape[0], X.shape[0]))

rth · 2018-11-27T19:51:14Z

sklearn/svm/tests/test_bounds.py

@@ -61,8 +61,8 @@ def check_l1_min_c(X, y, loss, fit_intercept=True, intercept_scaling=None):

    clf.C = min_c * 1.01
    clf.fit(X, y)
-    assert_true((np.asarray(clf.coef_) != 0).any() or
-                (np.asarray(clf.intercept_) != 0).any())
+    assert (np.asarray(clf.coef_) != 0).any() or \


Better to use the parenthesis here I think,

assert ((np.asarray(clf.coef_) != 0).any() or (np.asarray(clf.intercept_) != 0).any())

rth · 2018-11-27T19:54:26Z

sklearn/utils/tests/test_metaestimators.py

-        hasattr(MetaEstTestList(HasPredict(), HasPredict()), 'predict'))
+    assert not hasattr(MetaEst(HasNoPredict()), 'predict')
+    assert not \
+        hasattr(MetaEstTestTuple(HasNoPredict(), HasNoPredict()), 'predict')


also,

assert not hasattr(MetaEstTestTuple(HasNoPredict(), HasNoPredict()), 'predict')

or something similar if this is too long.

rth · 2018-11-27T19:54:35Z

sklearn/utils/tests/test_metaestimators.py

+    assert hasattr(MetaEstTestTuple(HasPredict(), HasNoPredict()), 'predict')
+    assert not \
+        hasattr(MetaEstTestTuple(HasNoPredict(), HasPredict()), 'predict')
+    assert not \


Same as above

rth · 2018-11-27T19:56:41Z

sklearn/utils/tests/test_validation.py

+                X.dtype == X_checked.dtype and
+                X_checked.flags['C_CONTIGUOUS'] == X.flags['C_CONTIGUOUS'] and
+                X_checked.flags['F_CONTIGUOUS'] == X.flags['F_CONTIGUOUS']
+            ):


This is an unrelated change I think? I'm not convinced the new formatting is much better, better to revert this.

jaskola8 · 2018-11-27T21:14:24Z

Thank you for your feedback, should I apply changes you mentioned above or leave it as it is?

rth · 2018-11-27T21:19:00Z

The first one was optionally, but if you could fix the other ones it would be great.

Thanks again for contributing!

qinhanmin2014

thanks @jaskola8

…ert (scikit-learn#12588)

…with assert (scikit-learn#12588)" This reverts commit 13da08b.

…ert (scikit-learn#12588)

Replaced all occurrences of assert_true() and assert_false() with ass…

d8776e8

…ert and assert not, kept definitions of replaced functions. Tested with pytest, flake8 and by using make.

Fixed flake8 warnings.

84ad059

rth requested changes Nov 15, 2018

View reviewed changes

jaskola8 changed the title ~~[MRG] Replaced all occurrences of assert_true() and assert_false() with assert.~~ Replaced all occurrences of assert_true() and assert_false() with assert. Nov 26, 2018

jaskola8 changed the title ~~Replaced all occurrences of assert_true() and assert_false() with assert.~~ [WIP]Replaced all occurrences of assert_true() and assert_false() with assert. Nov 26, 2018

Made required changes (parenthesis, assert line breaks and test for m…

75e31ca

…embership cleanup) for first half of tests (including models_selection/tests/test_validation.py).

Finished code cleanup, dense_assert_ and spare_assert_ dropped.

1321bc5

jaskola8 changed the title ~~[WIP]Replaced all occurrences of assert_true() and assert_false() with assert.~~ [MRG]Replaced all occurrences of assert_true() and assert_false() with assert. Nov 27, 2018

rth approved these changes Nov 27, 2018

View reviewed changes

Applied minor visual changes.

1bfe7ab

qinhanmin2014 approved these changes Nov 28, 2018

View reviewed changes

qinhanmin2014 merged commit fa98a72 into scikit-learn:master Nov 28, 2018

jaskola8 deleted the issue12580 branch November 28, 2018 09:36

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

MNT Replaced all occurrences of assert_true and assert_false with ass…

13da08b

…ert (scikit-learn#12588)

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "MNT Replaced all occurrences of assert_true and assert_false …

a0fa28f

…with assert (scikit-learn#12588)" This reverts commit 13da08b.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "MNT Replaced all occurrences of assert_true and assert_false …

94d4973

…with assert (scikit-learn#12588)" This reverts commit 13da08b.

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019

MNT Replaced all occurrences of assert_true and assert_false with ass…

3c3688b

…ert (scikit-learn#12588)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG]Replaced all occurrences of assert_true() and assert_false() with assert. #12588

[MRG]Replaced all occurrences of assert_true() and assert_false() with assert. #12588

jaskola8 commented Nov 14, 2018

jnothman commented Nov 14, 2018

rth left a comment •

edited

jaskola8 commented Nov 15, 2018

qinhanmin2014 commented Nov 20, 2018

qinhanmin2014 commented Nov 27, 2018

jaskola8 commented Nov 27, 2018 •

edited

rth left a comment

rth Nov 27, 2018

rth Nov 27, 2018

rth Nov 27, 2018

rth Nov 27, 2018

rth Nov 27, 2018

jaskola8 commented Nov 27, 2018

rth commented Nov 27, 2018

qinhanmin2014 left a comment

[MRG]Replaced all occurrences of assert_true() and assert_false() with assert. #12588

[MRG]Replaced all occurrences of assert_true() and assert_false() with assert. #12588

Conversation

jaskola8 commented Nov 14, 2018

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

jnothman commented Nov 14, 2018

rth left a comment • edited

Choose a reason for hiding this comment

jaskola8 commented Nov 15, 2018

qinhanmin2014 commented Nov 20, 2018

qinhanmin2014 commented Nov 27, 2018

jaskola8 commented Nov 27, 2018 • edited

rth left a comment

Choose a reason for hiding this comment

rth Nov 27, 2018

Choose a reason for hiding this comment

rth Nov 27, 2018

Choose a reason for hiding this comment

rth Nov 27, 2018

Choose a reason for hiding this comment

rth Nov 27, 2018

Choose a reason for hiding this comment

rth Nov 27, 2018

Choose a reason for hiding this comment

jaskola8 commented Nov 27, 2018

rth commented Nov 27, 2018

qinhanmin2014 left a comment

Choose a reason for hiding this comment

rth left a comment •

edited

jaskola8 commented Nov 27, 2018 •

edited