Skip to content

TST Extend tests for scipy.sparse.*array in sklearn/preprocessing/tests/test_common.py#27164

Merged
OmarManzoor merged 11 commits intoscikit-learn:mainfrom
work-mohit:fix/preprocessing/tests/test_common.py
Oct 2, 2023
Merged

TST Extend tests for scipy.sparse.*array in sklearn/preprocessing/tests/test_common.py#27164
OmarManzoor merged 11 commits intoscikit-learn:mainfrom
work-mohit:fix/preprocessing/tests/test_common.py

Conversation

@work-mohit
Copy link
Copy Markdown
Contributor

@work-mohit work-mohit commented Aug 25, 2023

Towards #27090

@github-actions
Copy link
Copy Markdown

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

✔️ Linting Passed

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

Generated for commit: 1b74ae6. Link to the linter CI: here

bharatr21
bharatr21 reviewed Aug 25, 2023
View reviewed changes
@jjerphan jjerphan mentioned this pull request Aug 25, 2023
Closed
@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Sep 11, 2023

@work-mohit what do you think of the suggestion discussed in https://github.com/scikit-learn/scikit-learn/pull/27164/files#r1307213688?

@work-mohit
Copy link
Copy Markdown
Contributor Author

work-mohit commented Sep 12, 2023

@work-mohit what do you think of the suggestion discussed in https://github.com/scikit-learn/scikit-learn/pull/27164/files#r1307213688?

Well Thanks for pointing out ! Now fixed. Hope it good to merge :)

@work-mohit
Copy link
Copy Markdown
Contributor Author

work-mohit commented Sep 12, 2023

@work-mohit what do you think of the suggestion discussed in https://github.com/scikit-learn/scikit-learn/pull/27164/files#r1307213688?

Well Thanks for pointing out ! Now fixed. Hope it good to merge :)

numpy.VisibleDeprecationWarning: Please use .todense() instead .

Pipeline showing this error. ( You can check the build error for the code traceback).

@work-mohit
Copy link
Copy Markdown
Contributor Author

work-mohit commented Sep 13, 2023

@work-mohit what do you think of the suggestion discussed in https://github.com/scikit-learn/scikit-learn/pull/27164/files#r1307213688?

Well Thanks for pointing out ! Now fixed. Hope it good to merge :)

numpy.VisibleDeprecationWarning: Please use .todense() instead .

Pipeline showing this error. ( You can check the build error for the code traceback).

@OmarManzoor can you help me with this?

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 @work-mohit. I think these suggestions should solve the deprecation warning.

@work-mohit
Copy link
Copy Markdown
Contributor Author

work-mohit commented Sep 13, 2023

Thanks @OmarManzoor for the help :)

@glemaitre glemaitre self-requested a review September 13, 2023 20:41
glemaitre
glemaitre reviewed Sep 13, 2023
View reviewed changes
@glemaitre glemaitre self-requested a review September 29, 2023 20:44
@work-mohit
Copy link
Copy Markdown
Contributor Author

work-mohit commented Sep 30, 2023

@glemaitre I think now it's good to merge!

@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Oct 2, 2023

@glemaitre I think now it's good to merge!

We will need 2 approvals but I will make a final review.

@work-mohit work-mohit requested a review from OmarManzoor October 2, 2023 08:43
glemaitre
glemaitre approved these changes Oct 2, 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.

LGTM on my side. Maybe @OmarManzoor wants to review this one.

OmarManzoor
OmarManzoor approved these changes Oct 2, 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. Thanks @work-mohit

@OmarManzoor OmarManzoor merged commit 32e3975 into scikit-learn:main Oct 2, 2023
@work-mohit work-mohit deleted the fix/preprocessing/tests/test_common.py branch October 2, 2023 09:48
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@work-mohit @REDVM
TST Extend tests for scipy.sparse.*array in `sklearn/preprocessing/…
a96d3e6 
…tests/test_common.py` (scikit-learn#27164)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ogrisel ogrisel ogrisel left review comments

@Charlie-XIAO Charlie-XIAO Charlie-XIAO left review comments

@glemaitre glemaitre glemaitre approved these changes

@OmarManzoor OmarManzoor OmarManzoor approved these changes

+1 more reviewer

@bharatr21 bharatr21 bharatr21 left review comments

Reviewers whose approvals may not affect merge requirements

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.

6 participants

@work-mohit @ogrisel @glemaitre @bharatr21 @OmarManzoor @Charlie-XIAO