8000 MNT Replace pytest.warns(None) in test_neighbors by thomasjpfan · Pull Request #23142 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MNT Replace pytest.warns(None) in test_neighbors #23142

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 3 commits into from
Apr 19, 2022
Merged

MNT Replace pytest.warns(None) in test_neighbors #23142

merged 3 commits into from
Apr 19, 2022

Conversation

thomasjpfan
Copy link
Member
@thomasjpfan thomasjpfan commented Apr 15, 2022 •
edited
Loading

Reference Issues/PRs

Follow up to #22572

What does this implement/fix? Explain your changes.

This PR removes the use of pytest.warns in test_neighbors that was in pytest.parameterize.

@thomasjpfan thomasjpfan added No Changelog Needed Quick Review For PRs that are quick to review labels Apr 15, 2022
thomasjpfan
thomasjpfan commented Apr 17, 2022
View reviewed changes
sklearn/neighbors/tests/test_neighbors.py
if global_dtype == np.float32:
# Warning from Scipy
ExceptionToAssert = DeprecationWarning
warn_context_manager = pytest.warns(DeprecationWarning)
Copy link
Member Author

Choose a reason for hiding this comment

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

The CI that uses sets SKLEARN_RUN_FLOAT32_TESTS=1, has an older SciPy version so this test is not covered.

Copy link
Member

Choose a reason for hiding this comment

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

It should be covered now that we run all jobs in the azure cron job

jeremiedbb
jeremiedbb approved these changes Apr 19, 2022
View reviewed changes
Copy link
Member
@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM

@jeremiedbb
Copy link
Member

Imo the check for the specific warning of wminkowski should just be in a separate test but let's keep as is for now for simplicity.

@jeremiedbb jeremiedbb merged commit e61040c into scikit-learn:main Apr 19, 2022
@ogrisel ogrisel mentioned this pull request Apr 20, 2022
Closed
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Apr 29, 2022
@thomasjpfan @jjerphan
MNT Replace pytest.warns(None) in test_neighbors (scikit-learn#23142)
ee8a22d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
module:neighbors No Changelog Needed Quick Review For PRs that are quick to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thomasjpfan @jeremiedbb
0