TST replace pytest.warns(None) in test_label_propagation.py #22998
Conversation
This commit updates the fit(X, y) definition, in the docs, of the sklearn.covariance.EllipticEnvelope class.
The original definition stated that parameter X could be of type 'array-like' or 'sparse matrix', however,
an error would be thrown if a 'sparse matrix' is passed ("TypeError: A sparse matrix was passed, but dense
data is required."). To resolve this, 'sparse matrix' was removed from the list of types X could be, in the
docstring.
This commit is part of the process to update the docs on the sklearn.covariance.EllipticEnvelope class's fit(X, y) definition, as explained in issue #14613.
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
This commit is to resolve part of issue #22572, specifically, refactoring test_label_propagation. This is being done to update the file and address the deprecation warning from Pytest when using "pytest.warns(None)".
|
@thomasjpfan this is the PR for |
|
@Ben3940 I didn't notice that your PR #22997 was made from the main branch of your fork (this one too btw). We advise to make PRs from a feature branch instead. Here's a guide describing how to properly setup your dev environment: https://scikit-learn.org/dev/developers/contributing.html#how-to-contribute That said, you can leave this PR as is since we squash all the commits before merging anyway. |
|
I see, I thought at some point I would have to merge my changes to my fork's |
| @@ -151,14 +152,16 @@ def test_convergence_warning(): | |||
| assert mdl.n_iter_ == mdl.max_iter | |||
|
|
|||
| mdl = label_propagation.LabelSpreading(kernel="rbf", max_iter=500) | |||
| with pytest.warns(None) as record: | |||
| with warnings.catch_warnings(): | |||
| warnings.simplefilter("error") | |||
There was a problem hiding this comment.
We prefer to explicitely pass the warning category that motivated this check. Based on the content of this test, it seems that it's a ConvergenceWarning
| with warnings.catch_warnings(): | ||
| mdl.fit(X, y) |
There was a problem hiding this comment.
You need to add to error on warnings otherwise this is not checking that there's no warning.
you can look at this issue linked in this test to find which warning is expected here (#9292)
|
@jeremiedbb I have gone back and made a new branch ( I am assuming that we are still wanting to avoid working on my fork's |
|
If you already did that, I guess the simplest solution is that you open a new PR from your new branch and then close this one. |
Reference Issues/PRs
Part of issue #22572 (Specifically
sklearn/semi_supervised/tests/test_label_propagation.pypart)What does this implement/fix? Explain your changes.
Refactored
pytest.warns(None)to usewarnings.catch_warnings()andwarnings.simplefilter("error"). This was done because of a deprecation warning from Pytest for usingpytest.warns(None)