RF: autoreplace assert_true(...==...) with plain assert #12547

yarikoptic · 2018-11-08T02:10:47Z

It is desired since assert_true (not even assert_equal) provides no information on actual value which fails the test

verify/fix any possible incorrect substitution

Note: Command (git-sedi) recorded in the commit msg comes from my zshrc:

# Useful for quick refactoring
git-sedi() {
  git grep -l $1 | xargs sed -i -e "s/$1/$2/g"
}

It is desired since assert_true (not even assert_equal) provides no information on actual value which fails the test === Do not change lines below === { "chain": [], "cmd": "git-sedi 'assert_true(\$ *.* == .*\$ *)$' 'assert \\1'", "exit": 0, "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

amueller · 2018-11-08T04:20:05Z

invaid syntax ;)

amueller · 2018-11-08T04:20:59Z

sklearn/model_selection/tests/test_search.py



 def check_cv_results_keys(cv_results, param_keys, score_keys, n_cand):
    # Test the search.cv_results_ contains all the required results
    assert_array_equal(sorted(cv_results.keys()),
                       sorted(param_keys + score_keys + ('params',)))
-    assert_true(all(cv_results[key].shape == (n_cand,)
+    assert all(cv_results[key].shape == (n_cand,


invalid syntax here I think?

also I see that the assert change from the original, is not?

yeah -- the 'cons' of auto sed. will need to fix them up - didn't spot on a brief eye balling

=== Do not change lines below === { "chain": [], "cmd": "git-sedi 'assert_true( *\$.*\$ *) *$' 'assert \\1'", "exit": 0, "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

…t_true

qinhanmin2014

We also need to remove relevant imports

rth

+1 for removing unused imports, we could also do the same thing for assert_equal in a follow-up PR.

I would even suggest including this in 0.20.1 as it shouldn't affect users, but make understanding test failure logs easier.

There was an earlier discussion about the possibility of creating merge conflicts (can't find it anymore), but in this case, I think it's worth it.

qinhanmin2014 · 2018-11-10T01:43:21Z

Fine with 0.20.1, as long as we can finish it.

@rth Maybe we don't need to do the same thing for assert_equal? We can still get things like AssertionError: a != b if we use assert_equal.

amueller · 2018-11-10T21:06:20Z

I don't think there's a very strong incentive for assert_equal, so I wouldn't prioritize it?

amueller

would be nice to remove unused imports but that's manual work so I'd be fine with a separate PR.

stsouko · 2018-11-11T19:56:28Z

assert_true(all([not isinstance(est, RandomForestClassifier) for est in
                     eclf2.estimators_]))

checks like this not fixed.

qinhanmin2014 · 2018-11-12T03:28:24Z

@stsouko Yes, this PR is not complete. One issue for some assert_true is how to handle the message. I think a PR to remove other assert_true is welcomed.

jnothman · 2018-11-12T09:19:52Z

What's hard about handling the message? The assert statement allows for a message

qinhanmin2014 · 2018-11-12T13:30:27Z

What's hard about handling the message? The assert statement allows for a
message

You're right, apologies for the stupid comment.

@stsouko Would you like to submit a PR? Otherwise I'll open an issue.

stsouko · 2018-11-12T15:01:33Z

At this moment I did a #12513. This assertion refers to the tests of voting classifier module. I fixed this test in PR.

qinhanmin2014 · 2018-11-12T15:30:52Z

@stsouko please do it in a separate PR, thanks

stsouko · 2018-11-12T16:22:15Z

I can do full fix of voting tests. according to this conclusion. Is it OK?

qinhanmin2014 · 2018-11-12T16:31:44Z

It's better to fix a single issue in each PR.

stsouko · 2018-11-12T16:48:51Z

done.

…ybutton * upstream/master: FIX remove FutureWarning in _object_dtype_isnan and add test (scikit-learn#12567) DOC Add 's' to "correspond" in docs for Hamming Loss. (scikit-learn#12565) EXA Fix comment in plot-iris-logistic example (scikit-learn#12564) FIX stop words validation in text vectorizers with custom preprocessors / tokenizers (scikit-learn#12393) DOC Add skorch to related projects (scikit-learn#12561) MNT Don't change self.n_values in OneHotEncoder.fit (scikit-learn#12286) MNT Remove unused assert_true imports (scikit-learn#12560) TST autoreplace assert_true(...==...) with plain assert (scikit-learn#12547) DOC: add a testimonial from JP Morgan (scikit-learn#12555)

…#12547)

…ikit-learn into add_codeblock_copybutton * 'add_codeblock_copybutton' of https://github.com/thoo/scikit-learn: Move an extension under sphinx_copybutton/ Move css/js file under sphinxext/ Fix max_depth overshoot in BFS expansion of trees (scikit-learn#12344) TST don't test utils.fixes docstrings (scikit-learn#12576) DOC Fix typo (scikit-learn#12563) FIX Workaround limitation of cloudpickle under PyPy (scikit-learn#12566) MNT bare asserts (scikit-learn#12571) FIX incorrect error when OneHotEncoder.transform called prior to fit (scikit-learn#12443) Retrigger travis:max time limit error DOC: Clarify `cv` parameter description in `GridSearchCV` (scikit-learn#12495) FIX remove FutureWarning in _object_dtype_isnan and add test (scikit-learn#12567) DOC Add 's' to "correspond" in docs for Hamming Loss. (scikit-learn#12565) EXA Fix comment in plot-iris-logistic example (scikit-learn#12564) FIX stop words validation in text vectorizers with custom preprocessors / tokenizers (scikit-learn#12393) DOC Add skorch to related projects (scikit-learn#12561) MNT Don't change self.n_values in OneHotEncoder.fit (scikit-learn#12286) MNT Remove unused assert_true imports (scikit-learn#12560) TST autoreplace assert_true(...==...) with plain assert (scikit-learn#12547) DOC: add a testimonial from JP Morgan (scikit-learn#12555)

…#12547)

…it-learn#12547)" This reverts commit fb5d3a7.

…#12547)

yarikoptic mentioned this pull request Nov 8, 2018

2 test failures on Debian stable (stretch) amd64 #12548

Closed

amueller reviewed Nov 8, 2018

View reviewed changes

yarikoptic added 4 commits November 8, 2018 13:17

BF: fixed syntax error introduced by automatic substitution

3d0f5b9

[DATALAD RUNCMD] git-sedi 'assert_true( *$.*$ *) *$' 'a...

d82c10e

=== Do not change lines below === { "chain": [], "cmd": "git-sedi 'assert_true( *\$.*\$ *) *$' 'assert \\1'", "exit": 0, "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

BF: fixed up some incorrect replacements (due to multiline) for asser…

a6ae5a1

…t_true

FLAKE8: removed now present over indentation (no _true any longer)

c093072

qinhanmin2014 approved these changes Nov 9, 2018

View reviewed changes

rth reviewed Nov 9, 2018

View reviewed changes

amueller approved these changes Nov 10, 2018

View reviewed changes

qinhanmin2014 merged commit 362cb3b into scikit-learn:master Nov 11, 2018

qinhanmin2014 mentioned this pull request Nov 11, 2018

MNT Remove unused assert_true imports #12560

Merged

stsouko mentioned this pull request Nov 12, 2018

[MRG] voting classifier assert fixed. #12571

Merged

qinhanmin2014 mentioned this pull request Nov 13, 2018

Replace assert_true and assert_false with assert #12580

Closed

thoo pushed a commit to thoo/scikit-learn that referenced this pull request Nov 13, 2018

TST autoreplace assert_true(...==...) with plain assert (scikit-learn…

bb1a9c6

…#12547)

thoo pushed a commit to thoo/scikit-learn that referenced this pull request Nov 14, 2018

TST autoreplace assert_true(...==...) with plain assert (scikit-learn…

5a4794f

…#12547)

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Nov 14, 2018

TST autoreplace assert_true(...==...) with plain assert (scikit-learn…

146ff3d

…#12547)

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Nov 14, 2018

TST autoreplace assert_true(...==...) with plain assert (scikit-learn…

6084189

…#12547)

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

TST autoreplace assert_true(...==...) with plain assert (scikit-learn…

fb5d3a7

…#12547)

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

Revert "TST autoreplace assert_true(...==...) with plain assert (scik…

141c1ca

…it-learn#12547)" This reverts commit fb5d3a7.

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

Revert "TST autoreplace assert_true(...==...) with plain assert (scik…

4b9e331

…it-learn#12547)" This reverts commit fb5d3a7.

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

TST autoreplace assert_true(...==...) with plain assert (scikit-learn…

307339a

…#12547)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RF: autoreplace assert_true(...==...) with plain assert #12547

RF: autoreplace assert_true(...==...) with plain assert #12547

yarikoptic commented Nov 8, 2018 •

edited

Loading

amueller commented Nov 8, 2018

amueller Nov 8, 2018

eamanu Nov 8, 2018

yarikoptic Nov 8, 2018

qinhanmin2014 left a comment

rth left a comment •

edited

Loading

qinhanmin2014 commented Nov 10, 2018

amueller commented Nov 10, 2018

amueller left a comment

stsouko commented Nov 11, 2018 •

edited

Loading

qinhanmin2014 commented Nov 12, 2018

jnothman commented Nov 12, 2018 via email

qinhanmin2014 commented Nov 12, 2018

stsouko commented Nov 12, 2018

qinhanmin2014 commented Nov 12, 2018

stsouko commented Nov 12, 2018 •

edited

Loading

qinhanmin2014 commented Nov 12, 2018

stsouko commented Nov 12, 2018

RF: autoreplace assert_true(...==...) with plain assert #12547

RF: autoreplace assert_true(...==...) with plain assert #12547

Conversation

yarikoptic commented Nov 8, 2018 • edited Loading

amueller commented Nov 8, 2018

amueller Nov 8, 2018

Choose a reason for hiding this comment

eamanu Nov 8, 2018

Choose a reason for hiding this comment

yarikoptic Nov 8, 2018

Choose a reason for hiding this comment

qinhanmin2014 left a comment

Choose a reason for hiding this comment

rth left a comment • edited Loading

Choose a reason for hiding this comment

qinhanmin2014 commented Nov 10, 2018

amueller commented Nov 10, 2018

amueller left a comment

Choose a reason for hiding this comment

stsouko commented Nov 11, 2018 • edited Loading

qinhanmin2014 commented Nov 12, 2018

jnothman commented Nov 12, 2018 via email

qinhanmin2014 commented Nov 12, 2018

stsouko commented Nov 12, 2018

qinhanmin2014 commented Nov 12, 2018

stsouko commented Nov 12, 2018 • edited Loading

qinhanmin2014 commented Nov 12, 2018

stsouko commented Nov 12, 2018

yarikoptic commented Nov 8, 2018 •

edited

Loading

rth left a comment •

edited

Loading

stsouko commented Nov 11, 2018 •

edited

Loading

stsouko commented Nov 12, 2018 •

edited

Loading