-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
TST Extend tests for scipy.sparse.*array
in sklearn/preprocessing/tests/test_label.py
#27227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @Tialo
classes=[1, 2], | ||
threshold=0, | ||
) | ||
for csr_container in CSR_CONTAINERS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about splitting the two loops that are using spare containers into a separate test and then parametrizing that? I think these tests are not dependent on anything else within this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this loop also be moved into the added test test_label_binarizer_sparse_errors
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, forgot about it
@@ -350,8 +352,11 @@ def test_sparse_output_multilabel_binarizer(): | |||
assert_array_equal([1, 2, 3], mlb.classes_) | |||
assert mlb.inverse_transform(got) == inverse | |||
|
|||
with pytest.raises(ValueError): | |||
mlb.inverse_transform(csr_matrix(np.array([[0, 1, 1], [2, 0, 0], [1, 1, 0]]))) | |||
for csr_container in CSR_CONTAINERS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly you might also be able to move this into a separate test and parametrize that since this part is just checking for an error and is independent from the rest of the test.
@OmarManzoor Thanks for your comments. In second test I used MultiLabelBinarizer and input according to last values in for loop, because it was used in original test, if you have better solution, I will be glad to make a fix |
I think it looks fine considering it is replicating the original behavior. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @Tialo
@OmarManzoor if you want to have a second look at it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…tests/test_label.py` (scikit-learn#27227)
Reference Issues/PRs
Towards #27090.
What does this implement/fix? Explain your changes.
Any other comments?
I used for loops instead of parameterisation in some tests to save time and do not repeat other checks in test that do not depend on sparse type.
Also i fixed test
test_label_binarize_multilabel
. It used only lasty
from for loop inpytest.raises(ValueError)