Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

TST replace assert_raise_* by pytest.raises in neighbors module #19388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Feb 22, 2021
Merged

TST replace assert_raise_* by pytest.raises in neighbors module #19388

merged 6 commits into from
Feb 22, 2021

Conversation

Haidar13
Copy link
Contributor

@Haidar13 Haidar13 commented Feb 6, 2021 •
edited
Loading

Reference Issues/PRs

This PR is related to issue #14216

What does this implement/fix? Explain your changes.

Change assert_raises and assert_raises_regex to :

with pytest.raises(.....)

@Haidar13 @stevenkolawole
Change assert_raises into pytest.raises in sklearn/neighbors/test
d6702a2 
Co-authored-by: SteveKola <kolawolesteven99@gmail.com>
@reshamas
Copy link
Member

reshamas commented Feb 6, 2021

#DataUmbrella sprint
cc: @Mariam-ke

@glemaitre glemaitre self-requested a review February 7, 2021 09:49
glemaitre
glemaitre reviewed Feb 7, 2021
View reviewed changes
Copy link
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.

Could you in addition check the following file to replace assert_raise_message by the the pytest.raises context manager as well:

  • test_optics.py
  • test_mean_shift.py
  • test_hierarchical.py

sklearn/neighbors/tests/test_nca.py Outdated Show resolved Hide resolved
sklearn/neighbors/tests/test_neighbors.py Show resolved Hide resolved
@glemaitre glemaitre changed the title [MRG] Change assert_raises to pytest.raises in sklearn/neighbors/test TST replace assert_raise_* by pytest.raises in neighbors module Feb 7, 2021
@Haidar13
Copy link
Contributor Author

@glemaitre all raise_message and raise_warn have been changed to the with pytest context

@reshamas
Copy link
Member

cc: @SteveKola

@Haidar13 Would you please remove Steven's email address from the PR description at the top? I tagged his GitHub handle here. Thank you.

@reshamas reshamas mentioned this pull request Feb 15, 2021
Closed
@Haidar13
Copy link
Contributor Author

cc: @SteveKola

@Haidar13 Would you please remove Steven's email address from the PR description at the top? I tagged his GitHub handle here. Thank you.

I removed the email address,
Thanks

ogrisel
ogrisel reviewed Feb 18, 2021
View reviewed changes
Copy link
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.

I introduced spurious changes when resolving the conflicts when merging the main branch. Let me undo those.

sklearn/cluster/tests/test_affinity_propagation.py Outdated Show resolved Hide resolved
sklearn/cluster/tests/test_affinity_propagation.py Outdated Show resolved Hide resolved
sklearn/cluster/tests/test_hierarchical.py Outdated Show resolved Hide resolved
sklearn/cluster/tests/test_hierarchical.py Outdated Show resolved Hide resolved
ogrisel
ogrisel reviewed Feb 18, 2021
View reviewed changes
Copy link
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.

Minor style comment. Otherwise, looks good to me.

sklearn/neighbors/tests/test_nca.py Outdated Show resolved Hide resolved
ogrisel
ogrisel approved these changes Feb 20, 2021
View reviewed changes
Copy link
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!

@Haidar13
Copy link
Contributor Author

@ogrisel
@glemaitre

Thank you for spending time reviewing my PR.
Looking forward to contributing more

@glemaitre glemaitre merged commit c3eb5ed into scikit-learn:main Feb 22, 2021
@glemaitre
Copy link
Member

Thanks @Haidar13 It looks good.

@lorentzenchr lorentzenchr mentioned this pull request Mar 2, 2021
Closed
43 tasks
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre left review comments

@ogrisel ogrisel ogrisel approved these changes

Assignees
No one assigned
Labels
module:neighbors Waiting for Reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Haidar13 @reshamas @glemaitre @ogrisel
Morty Proxy This is a proxified and sanitized view of the page, visit original site.