TST Fix scipy DeprecationWarning from wminkowski in nightly #18930

cmarmo · 2020-11-27T11:02:15Z

What does this implement/fix? Explain your changes.

This pull request modifies tests to use 'minkoski' metrics when 'wminkowski' is called when scipy version is >= 1.6.
This remove some DeprecationWarnings in the nightly build.

Any other comments?

I'm under the impression that the minkoski metrics is already called with a 'w' argument

scikit-learn/sklearn/metrics/tests/test_pairwise.py

Line 236 in eaa45c8

_wminkowski_kwds = {'w': np.arange(1, 5).astype('double', copy=False), 'p': 1}
Should I raise a sklearn DeprecationWarning for 'wminkowski' metric (called in sklearn/metrics/pairwise.py) or we can rely on the scipy one? Thanks!

…al to 1.6.

sklearn/metrics/tests/test_pairwise.py

ogrisel · 2020-11-27T16:00:22Z

Should I raise a sklearn DeprecationWarning for 'wminkowski' metric (called in sklearn/metrics/pairwise.py) or we can rely on the scipy one? Thanks!

I think we can rely on scipy one for simplicity. Maybe we could upgrade the test to also check minkowski with the w parameter in versions before 1.6.

I am running scipy 1.5 and this w parameter is already present.

ogrisel · 2020-11-30T18:04:08Z

The second part of my comment was not addressed:

Maybe we could upgrade the test to also check minkowski with the w parameter in versions before 1.6. I am running scipy 1.5 and this w parameter is already present.

Can you give it a try?

The oldest version of scipy we support (0.19.1 as defined in sklearn._min_dependencies) does not have the w parameter for the minkowski function but scipy 1.0 and later do. So we could just skip this particular test parametrization for sp_version < parse_version("1.0") using an explicit call to pytest.skip such as: pytest.skip("minkowski does not accept the w parameter prior to scipy 1.0").

cmarmo · 2020-11-30T18:14:08Z

I'm sorry @ogrisel, I was having a blind spot on the w. I see now that the arguments are not defined for minkowsky, they are only for wminkowsky. I will add the test ASAP.

ogrisel

LGTM!

ogrisel · 2020-12-15T17:47:51Z

We can ignore the coverage failure because all the cases cannot be tested prior to the scipy 1.6 release but the code looks correct. And even if it is not, the nightly builds will catch it quickly after the merge of this PR.

thomasjpfan

Thank you for the PR @cmarmo !

thomasjpfan · 2020-12-17T19:36:49Z

sklearn/metrics/tests/test_pairwise.py

+except ImportError:
+    # In scipy 1.6.0, wminkowski is deprecated and minkowski
+    # should be used instead.
+    from scipy.spatial.distance import minkowski as wminkowski


As for coverage, the ImportError will not trigger until scipy 1.8 is released and wminkowski is removed.

(This is a comment: no changes required here)

thomasjpfan · 2020-12-17T19:39:37Z

sklearn/metrics/tests/test_pairwise.py

+    if sp_version >= parse_version("1.6.0") and (
+        metric == wminkowski or metric == 'wminkowski'
+    ):
+        metric = 'minkowski'


In this case, we can skip because minkowski is tested explicitly and this would be testing it again.

I'm not sure to understand correctly: I have then skipped the wminkowski and 'wminkowski' test for scipy >=1.6.0, Is that what you meant? Thanks!

I have then skipped the wminkowski and 'wminkowski' test for scipy >=1.6.0, Is that what you meant?

Yes this is what I meant.

I think we can skip here because this PR adds metric = 'minkowski' to pytest.mark.parameterize, so metric = 'minkowski' will get tested when it gets to that test case.

thomasjpfan · 2020-12-17T19:41:33Z

sklearn/metrics/tests/test_pairwise.py

@@ -245,13 +254,26 @@ def callable_rbf_kernel(x, y, **kwds):
 @pytest.mark.parametrize(
        'func, metric, kwds',
        [(pairwise_distances, 'euclidean', {}),
+         (pairwise_distances, minkowski, _minkowski_kwds),


We can include the skip here:

Suggested change

(pairwise_distances, minkowski, _minkowski_kwds),

pytest.param(pairwise_distances, minkowski, _minkowski_kwds,

marks=pytest.mark.skipif(sp_version < parse_version("1.0")),

reason="minkoski does not accept ..."),

I guess this kind of parametrization should be applied to the 'minkowsi' case too, right?

thomasjpfan

LGTM

thomasjpfan · 2021-01-02T14:21:24Z

Currently, the nightly build is not able to run without #19062 merged

…earn#18930)

Use minkowsi in place of wminkowski when scipy is greater than or equ…

6c62d6c

…al to 1.6.

github-actions bot added the module:metrics label Nov 27, 2020

Fix lint error.

d9dbacc

ogrisel reviewed Nov 27, 2020

View reviewed changes

sklearn/metrics/tests/test_pairwise.py Outdated Show resolved Hide resolved

sklearn/metrics/tests/test_pairwise.py Outdated Show resolved Hide resolved

sklearn/metrics/tests/test_pairwise.py Outdated Show resolved Hide resolved

cmarmo added 2 commits November 27, 2020 17:42

Address comments.

8bc4ba2

Revert removed blank lines.

a3cb208

cmarmo added 2 commits November 30, 2020 22:36

Add test for minkowski metric: to be skipped is scipy < 1.0.

06e437a

Merge branch 'master' into scipy-wminkowski

7e21735

cmarmo added the ci sync label Dec 1, 2020

github-actions bot removed the ci sync label Dec 1, 2020

ogrisel mentioned this pull request Dec 1, 2020

MNT remove .github/workflows/sync_pull_request.yml #18951

Merged

Merge branch 'master' into scipy-wminkowski

fd5475a

ogrisel approved these changes Dec 15, 2020

View reviewed changes

ogrisel added the Waiting for Reviewer label Dec 15, 2020

thomasjpfan reviewed Dec 17, 2020

View reviewed changes

cmarmo removed the Waiting for Reviewer label Dec 18, 2020

cmarmo added 5 commits December 18, 2020 12:42

Move test marks in parametrization.

0c73080

Lint.

d534255

Skip doubled test for scipy > 1.6.0.

ca2e8fa

ERRATA CORRIGE: Skip doubled test for scipy > 1.6.0.

4ab74fb

Skip test on scipy version in the code.

b0fe75a

cmarmo added the Waiting for Reviewer label Dec 18, 2020

cmarmo mentioned this pull request Jan 1, 2021

FIX Fix recall in multilabel classification when true labels are all negative #19085

Merged

thomasjpfan approved these changes Jan 2, 2021

View reviewed changes

thomasjpfan changed the title ~~Fix scipy DeprecationWarning from wminkowski in nightly build~~ TST Fix scipy DeprecationWarning from wminkowski in nightly Jan 2, 2021

thomasjpfan merged commit b7a48a0 into scikit-learn:master Jan 2, 2021

getgaurav2 mentioned this pull request Jan 4, 2021

FEA Group aware Time-based cross validation #16236

Open

cmarmo deleted the scipy-wminkowski branch January 10, 2021 17:34

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Jan 19, 2021

TST Fix scipy DeprecationWarning from wminkowski in nightly (scikit-l…

4c3f0dc

…earn#18930)

jeremiedbb pushed a commit that referenced this pull request Jan 19, 2021

TST Fix scipy DeprecationWarning from wminkowski in nightly (#18930)

3fea406

dependabot bot mentioned this pull request Mar 12, 2021

Bump scikit-learn from 0.23.1 to 0.24.1 JadHADDAD92/covid-mask-detector#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST Fix scipy DeprecationWarning from wminkowski in nightly #18930

TST Fix scipy DeprecationWarning from wminkowski in nightly #18930

cmarmo commented Nov 27, 2020

ogrisel commented Nov 27, 2020

ogrisel commented Nov 30, 2020

cmarmo commented Nov 30, 2020

ogrisel left a comment

ogrisel commented Dec 15, 2020

thomasjpfan left a comment

thomasjpfan Dec 17, 2020

thomasjpfan Dec 17, 2020

cmarmo Dec 18, 2020

thomasjpfan Dec 18, 2020

thomasjpfan Dec 17, 2020

cmarmo Dec 18, 2020

thomasjpfan left a comment

thomasjpfan commented Jan 2, 2021

-         (pairwise_distances, minkowski, _minkowski_kwds),
+         pytest.param(pairwise_distances, minkowski, _minkowski_kwds,
+         			  marks=pytest.mark.skipif(sp_version < parse_version("1.0")),
+         			  reason="minkoski does not accept ..."),

TST Fix scipy DeprecationWarning from wminkowski in nightly #18930

TST Fix scipy DeprecationWarning from wminkowski in nightly #18930

Conversation

cmarmo commented Nov 27, 2020

What does this implement/fix? Explain your changes.

Any other comments?

ogrisel commented Nov 27, 2020

ogrisel commented Nov 30, 2020

cmarmo commented Nov 30, 2020

ogrisel left a comment

Choose a reason for hiding this comment

ogrisel commented Dec 15, 2020

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan Dec 17, 2020

Choose a reason for hiding this comment

thomasjpfan Dec 17, 2020

Choose a reason for hiding this comment

cmarmo Dec 18, 2020

Choose a reason for hiding this comment

thomasjpfan Dec 18, 2020

Choose a reason for hiding this comment

thomasjpfan Dec 17, 2020

Choose a reason for hiding this comment

cmarmo Dec 18, 2020

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan commented Jan 2, 2021