Skip to content

TST Extend tests for scipy.sparse/*array in sklearn/impute/tests/test_common#27277

Merged
glemaitre merged 10 commits intoscikit-learn:mainfrom
Charlie-XIAO:tst_sp_impute_common
Sep 15, 2023
Merged

TST Extend tests for scipy.sparse/*array in sklearn/impute/tests/test_common#27277
glemaitre merged 10 commits intoscikit-learn:mainfrom
Charlie-XIAO:tst_sp_impute_common

Conversation

@Charlie-XIAO
Copy link
Copy Markdown
Contributor

Towards #27090.

I made the .tocsr() change because scipy.sparse.hstack would cause the output to become coo_matrix when input is sparse array, and coo_matrix does not support slicing.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 3, 2023

✔️ Linting Passed

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

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

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. Some minor comments.

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. Thanks @Charlie-XIAO

@glemaitre glemaitre self-requested a review September 11, 2023 17:41
@Charlie-XIAO
Copy link
Copy Markdown
Contributor Author

@glemaitre I've specified the format in sp.hstack to be the same as that of X_imputer so that the behavior of sparse arrays and matrices (of the same format) should be consistent now. Do we need a changelog for this and if so, what should we say?

@glemaitre
Copy link
Copy Markdown
Member

Nice. Indeed, we need a changelog entry as well.

@glemaitre glemaitre self-requested a review September 15, 2023 19:41
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.

LGTM

@glemaitre glemaitre merged commit 8faa920 into scikit-learn:main Sep 15, 2023
@Charlie-XIAO Charlie-XIAO deleted the tst_sp_impute_common branch September 16, 2023 02:09
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
…est_common` (scikit-learn#27277)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
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