8000 TST Extend tests for `scipy.sparse.*array` in `sklearn/preprocessing/tests/test_label.py` by Tialo · Pull Request #27227 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

Tialo
Copy link
Contributor
@Tialo Tialo commented Aug 30, 2023

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 last y from for loop in pytest.raises(ValueError)

@github-actions
Copy link
github-actions bot commented Aug 30, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: f707162. Link to the linter CI: here

Copy link
Contributor
@OmarManzoor OmarManzoor left a 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:
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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:
Copy link
Contributor

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.

@Tialo
Copy link
Contributor Author
Tialo commented Sep 1, 2023

@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

@OmarManzoor
Copy link
Contributor

@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. 👍

@glemaitre glemaitre self-requested a review September 13, 2023 12:32
Copy link
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.

LGTM. Thanks @Tialo

@glemaitre
Copy link
Member
glemaitre commented Sep 13, 2023 8000

@OmarManzoor if you want to have a second look at it.

Copy link
Contributor
@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

LGTM

@OmarManzoor OmarManzoor merged commit 1fffa8d into scikit-learn:main Sep 13, 2023
@Tialo Tialo deleted the tests/test_label branch September 13, 2023 12:51
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0