Skip to content

TST Extend tests for scipy.sparse.*array in sklearn/linear_model/tests/test_perceptron.py#27160

Merged
ogrisel merged 5 commits intoscikit-learn:mainfrom
IsaacTrost:main
Aug 28, 2023
Merged

TST Extend tests for scipy.sparse.*array in sklearn/linear_model/tests/test_perceptron.py#27160
ogrisel merged 5 commits intoscikit-learn:mainfrom
IsaacTrost:main

Conversation

@IsaacTrost
Copy link
Copy Markdown
Contributor

@IsaacTrost IsaacTrost commented Aug 25, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Towards #27090. Added sparce array tests to a test file.

No changelog should be necessary for this fix.

Any other comments?

As per the issue I tackled, this is my first time contributing to this repo. I believe I did everything the issue asked for, though I am unsure.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 25, 2023

✔️ Linting Passed

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

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

@OmarManzoor OmarManzoor changed the title TST Extend tests for scipy.sparse.*array in <filename> TST Extend tests for scipy.sparse.*array in sklearn/linear_model/tests/test_perceptron.py Aug 25, 2023
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! Here are a few suggestions.

Copy link
Copy Markdown
Member

@ogrisel ogrisel 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, please see the comments below:

@IsaacTrost
Copy link
Copy Markdown
Contributor Author

Thanks so much for the suggestions! Everything should be fixed up now, let me know if I need to do anything else.

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 @IsaacTrost. LGTM.

Copy link
Copy Markdown
Member

@ogrisel ogrisel 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 for the PR and thanks for taking our feedback into account!

@ogrisel ogrisel merged commit 952f480 into scikit-learn:main Aug 28, 2023
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Aug 29, 2023
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