Skip to content

TST Extend tests for scipy.sparse/*array in sklearn/preprocessing/tests/test_data#27253

Merged
glemaitre merged 7 commits intoscikit-learn:mainfrom
Charlie-XIAO:tst_sp_data
Sep 13, 2023
Merged

TST Extend tests for scipy.sparse/*array in sklearn/preprocessing/tests/test_data#27253
glemaitre merged 7 commits intoscikit-learn:mainfrom
Charlie-XIAO:tst_sp_data

Conversation

@Charlie-XIAO
Copy link
Copy Markdown
Contributor

@Charlie-XIAO Charlie-XIAO commented Aug 31, 2023 •
edited
Loading

Towards #27090.

There seems to be many duplicated code and even wrong tests, so I kind of refactored it a bit (fixed those errors and duplicates that I noticed).

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 31, 2023 •
edited
Loading

✔️ Linting Passed

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

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

@jjerphan jjerphan mentioned this pull request Aug 31, 2023
Closed
@glemaitre glemaitre self-requested a review September 12, 2023 08:48
glemaitre
glemaitre reviewed Sep 12, 2023
View reviewed changes
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 the changes looks good. The parametrization allows to remove some legacy code. Thanks.

I also added in the review comments that should be avoiding the CIs failures.

@Charlie-XIAO
Copy link
Copy Markdown
Contributor Author

Charlie-XIAO commented Sep 12, 2023

Thanks @glemaitre for the review! Thank you so much for the effort finding out every place where I used unpacking and those deprecated .A usages. I've made the suggested changes, and there is one question left in #27253 (comment)

glemaitre
glemaitre approved these changes Sep 13, 2023
View reviewed changes
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.

The additional check is exactly what I meant. Thanks @Charlie-XIAO. LGTM.

@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Sep 13, 2023

@OmarManzoor This one will take a bit more time to review but this is in a good state.

OmarManzoor
OmarManzoor reviewed Sep 13, 2023
View reviewed changes
Copy link
Copy Markdown
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 @Charlie-XIAO

@Charlie-XIAO
Copy link
Copy Markdown
Contributor Author

Charlie-XIAO commented Sep 13, 2023

Thanks for spotting those unpacking stuff @OmarManzoor! I missed those when I checked through my PRs. Also I left a question in #27253 (comment) about whether combination is necessary.

OmarManzoor
OmarManzoor approved these changes Sep 13, 2023
View reviewed changes
Copy link
Copy Markdown
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.

@glemaitre glemaitre merged commit 9c46a6c into scikit-learn:main Sep 13, 2023
@OmarManzoor
Copy link
Copy Markdown
Contributor

OmarManzoor commented Sep 13, 2023

Thanks for the updates @Charlie-XIAO

@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Sep 13, 2023

Merging since this is passing. Thanks all

@Charlie-XIAO Charlie-XIAO deleted the tst_sp_data branch September 14, 2023 03:57
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@Charlie-XIAO @OmarManzoor @REDVM
TST Extend tests for scipy.sparse/*array in `sklearn/preprocessing/…
e43cf4c 
…tests/test_data` (scikit-learn#27253)

Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glemaitre glemaitre glemaitre approved these changes

@OmarManzoor OmarManzoor OmarManzoor approved these changes

Assignees

No one assigned

Labels

module:preprocessing No Changelog Needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Charlie-XIAO @glemaitre @OmarManzoor