TST replace assert_raises* by pytest.raises in model_selection/tests/test_split.py #19375
Conversation
|
#DataUmbrella sprint |
|
We have an issue with the linter: https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=26247&view=logs&jobId=32e2e1bb-a28f-5b18-6cfc-3f01273f5609&j=32e2e1bb-a28f-5b18-6cfc-3f01273f5609&t=fc67071d-c3d4-58b8-d38e-cafc0d3c731a Could you check to wrap-up the lines with 79 characters. |
|
of course |
fb1d8cb to
bf95d03
Compare
There was a problem hiding this comment.
LGTM overall. +1 for merging this with or without @lorentzenchr's comment above and the following style nitpicks.
| with pytest.raises( | ||
| ValueError, | ||
| match="Cannot have number of splits.*greater" | ||
| ): | ||
| next(GroupKFold(n_splits=3).split(X, y, groups)) |
There was a problem hiding this comment.
nitpick, I think the future indenting style we are converging to for the scikit-learn project for such multiline expressions would rather be the following (@glemaitre correct me if I am wrong):
| with pytest.raises( | |
| ValueError, | |
| match="Cannot have number of splits.*greater" | |
| ): | |
| next(GroupKFold(n_splits=3).split(X, y, groups)) | |
| with pytest.raises( | |
| ValueError, | |
| match="Cannot have number of splits.*greater" | |
| ): | |
| next(GroupKFold(n_splits=3).split(X, y, groups)) |
| with pytest.raises( | ||
| ValueError, | ||
| match="Cannot have number of folds.*greater" | ||
| ): |
Sure |
|
I would be in favour of the |
| with pytest.raises( | ||
| ValueError, | ||
| match="Cannot have number of folds.*greater" | ||
| ): |
There was a problem hiding this comment.
FYI
| with pytest.raises( | |
| ValueError, | |
| match="Cannot have number of folds.*greater" | |
| ): | |
| with pytest.raises( | |
| ValueError, | |
| match="Cannot have number of folds.*greater" | |
| ): |
|
I'll continue working on this PR. |
|
Due to lack of permissions to push to this PR branch, a new branch starting from this PR branch is created. #19585 is a new PR with fixes to the comments raised here. |
|
Resolved in #19585 . Thanks! |
Reference Issues/PRs
References #14216
What does this implement/fix? Explain your changes.
Removes the use of assert_raises* in model_selection/tests/test_split.py
Any other comments?
cc: (pair programming partner) @mohaseeb