Skip to content

TST: Fix assert raises in sklearn/metrics/tests/#14715

Merged
rth merged 14 commits intoscikit-learn:masterfrom
sameshl:tests_metrics
Sep 8, 2019
Merged

TST: Fix assert raises in sklearn/metrics/tests/#14715
rth merged 14 commits intoscikit-learn:masterfrom
sameshl:tests_metrics

Conversation

@sameshl
Copy link
Copy Markdown
Contributor

@sameshl sameshl commented Aug 22, 2019

replaced assert_raises and assert_raises_regex with pytest.raises context manager.

related to #14216

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

Same as other PRs: remove assert_raise_message.


# use context manager to supply custom error message
with assert_raises(AssertionError) as cm:
with pytest.raises(AssertionError) as cm:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will not work as intended with pytest.

I think that we want something like this instead

with pytest.raises(AssertionError):
    assert_array_equal(metric(y_true, y_pred), metric(y_pred, y_true))
    raise ValueError("%s seems to be symmetric:" %name)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@glemaitre Thanks for pointing that out! Will make the required change.


# use context manager to supply custom error message
with assert_raises(AssertionError) as cm:
with pytest.raises(AssertionError) as cm:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is the same issue than above. You need to raise a ValueError within the context manager.

assert_raises_regex(ValueError, "Mean Squared Logarithmic Error cannot be "
"used when targets contain negative values.",
mean_squared_log_error, [1., -2., 3.], [1., 2., 3.])
with pytest.raises(ValueError, match="Mean Squared Logarithmic Error "
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can move the message out of raises

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

Otherwise LGTM. We should open something to refactor these tests with parametrization.

@sameshl
Copy link
Copy Markdown
Contributor Author

sameshl commented Aug 29, 2019

@rth. I have made the required changes. You can have a look.

@sameshl
Copy link
Copy Markdown
Contributor Author

sameshl commented Sep 8, 2019

@rth I have removed all except one instances of re.escape and removed them by manually escaping the string. The one I am not able to do is dynamically escape the string. Can you help me out on that?

@sameshl
Copy link
Copy Markdown
Contributor Author

sameshl commented Sep 8, 2019

@thomasjpfan Can you have a look?

@rth rth dismissed glemaitre’s stale review September 8, 2019 12:09

Comments were addressed

Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @sameshl !

@rth rth changed the title MAINT:Fix assert raises in sklearn/metrics/tests/ TST: Fix assert raises in sklearn/metrics/tests/ Sep 8, 2019
@rth rth merged commit 538546b into scikit-learn:master Sep 8, 2019
@sameshl sameshl deleted the tests_metrics branch September 8, 2019 14:05
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.

3 participants