Skip to content

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

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

glemaitre merged 10 commits into
scikit-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

github-actions Bot commented Sep 3, 2023 •

Copy link
Copy Markdown

✔️ Linting Passed

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

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

@OmarManzoor OmarManzoor left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread sklearn/impute/tests/test_common.py Outdated
Comment thread sklearn/impute/tests/test_common.py Outdated

@OmarManzoor OmarManzoor left a comment

Copy link
Copy Markdown
Contributor

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
Comment thread sklearn/impute/tests/test_common.py Outdated
@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

@glemaitre glemaitre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread doc/whats_new/v1.4.rst Outdated
Comment thread doc/whats_new/v1.4.rst Outdated
@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