FIX Sets dtype for pairwise_distances when metric='seuclidean' #15730

ForrestCKoch · 2019-11-28T03:49:20Z

Reference Issues/PRs

Please see my issue at #15731

What does this implement/fix? Explain your changes.

sklearn.metrics.pairwise_distances returns error when using metric='seuclidean' and input is not of type np.double.

Any other comments?

This is my first contribution to scikit-learn. I have read through the contributions page, but please let me know if I have done anything wrong.

jnothman

Please add a test

jeremiedbb · 2019-11-28T11:05:07Z

Looks like a scipy bug to me. You can pass a float32 array to scipy.spatial.distance.pdist for X but not for V when metric="seuclidean". However you can pass a float32 array for VI when metric="mahalanobis".

…oats

ForrestCKoch · 2019-11-29T00:27:34Z

@jnothman

Please add a test

Sure thing -- I've added a test to compare the output of pairwise_distances with pdist and cdist for each of the numpy floating point datatypes.

@jeremiedbb

Looks like a scipy bug to me. You can pass a float32 array to scipy.spatial.distance.pdist for X but not for V when metric="seuclidean". However you can pass a float32 array for VI when metric="mahalanobis".

Yeah, I'm not really sure what they were thinking (see the code snippet below). They force conversion to np.double at the end of their argument validation anyways, so I'm not sure what they were hoping to acheive with this check.

def _validate_seuclidean_kwargs(X, m, n, **kwargs):
    V = kwargs.pop('V', None)
    if V is None:
        V = np.var(X.astype(np.double), axis=0, ddof=1)
    else:
        V = np.asarray(V, order='c')
        if V.dtype != np.double:
            raise TypeError('Variance vector V must contain doubles.')
        if len(V.shape) != 1:
            raise ValueError('Variance vector V must '
                             'be one-dimensional.')
        if V.shape[0] != n:
            raise ValueError('Variance vector V must be of the same '
                             'dimension as the vectors on which the distances '
                             'are computed.')
    kwargs['V'] = _convert_to_double(V)

Regardless, I think this fix is required in the meantime. If they remove this forced conversion to np.double, the tests I have written will fail due to the difference in precision float types, and my changes can be reverted to avoid the performance penalty of type conversion.

jeremiedbb · 2019-12-03T09:13:29Z

Yeah, I'm not really sure what they were thinking (see the code snippet below). They force conversion to np.double at the end of their argument validation anyways, so I'm not sure what they were hoping to acheive with this check.

Yes the check for the dtype is weird. the validation of mahalanobis params is the same except for this dtype check. We should report this to scipy.

Regardless, I think this fix is required in the meantime

that's right.

jeremiedbb

Some comments below. Also please add a what's new entry.

jeremiedbb · 2019-12-03T09:14:27Z

sklearn/metrics/tests/test_pairwise.py

@@ -1273,3 +1273,27 @@ def test_pairwise_distances_data_derived_params(n_jobs, metric, dist_function,

        assert_allclose(dist, expected_dist_explicit_params)
        assert_allclose(dist, expected_dist_default_params)
+
+
+@pytest.mark.parametrize("metric", ["seuclidean"])


You don't need to parametrize for a single value

Sorry, I had originally tried to include 'mahalanobis' in the tests, but it kept failing because one of the Windows test benches didn't like me trying to use np.double.

It was throwing an error in one of the np.linalg libraries, so I removed it as I didn't think it was necessary to test for this purpose.

Anyways -- This is fixed in my recent commit

sklearn/metrics/tests/test_pairwise.py

jeremiedbb · 2019-12-03T09:23:46Z

sklearn/metrics/tests/test_pairwise.py

+    # check that pairwise distances gives the same result as pdist and cdist
+    # regardless of input datatype


Please mention that the seuclidean metric used to raise an error on non double dtype, and add the PR number.

Also fixed in recent commit

jeremiedbb · 2019-12-04T13:50:33Z

I opened scipy/scipy#11171 to fix this issue upstream

Co-Authored-By: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

ForrestCKoch · 2019-12-05T04:36:21Z

@jeremiedbb thanks for opening that issue with scipy.

I've added a new "What's new" entry; however, I'm not very confident that I've gotten the syntax/conventions right. I've tried to mimic the other entries, but let me know if there are any mistakes.

jeremiedbb

Just a last nitpick from my side. Otherwise looks good. Thanks @ForrestCKoch

sklearn/metrics/tests/test_pairwise.py

Co-Authored-By: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

jnothman · 2019-12-05T05:08:24Z

sklearn/metrics/tests/test_pairwise.py

+        "dtype",
+        [np.half, np.float, np.double, np.longdouble])
+@pytest.mark.parametrize("y_is_x", [True, False], ids=["Y is X", "Y is not X"])
+def test_pairwise_distances_input_datatypes(dtype, y_is_x):


This should probably mention seuclidean as it only tests that metric.

Or, rather than adding a test, we should be modifying another test to ensure invariance to dtype across metrics

This should probably mention seuclidean as it only tests that metric.

To clarify, you mean I should change the name of the test? Otherwise it is mentioned in the following comments.

Or, rather than adding a test, we should be modifying another test to ensure invariance to dtype across metrics

I had originally tried to extend this to include other metrics, but 'malahanobis' will fail on the Windows py35_pip_openblas_32bit benchmark because np.linalg doesn't like the usage of np.double presumably due to the 32bit openblas library.

Then it might be best to test on all but xfail on some particularly problematic combinations.

At the moment it looks quite strange that you are only testing seuclidean here

Please see my most recent revisions. I've added each of the non-boolean scipy distance metrics to the tests. xfails have been introduced if each of the following hold:

metric == 'mahalanobis'

dtype == np.longdouble

sys.platform.startswith('win')

…to be xfail

…=='euclidean'

ForrestCKoch · 2020-07-14T12:06:17Z

@jeremiedbb -- I only applied the tolerance adjustment for the float32 and metric=='cosine' test because this is the only one that was failing. Let me know if you would rather it be applied to all float32 cases and I'll make that adjustment.

Also, I found a couple of minor typos which I corrected in becdd36, if this is not okay I'll revert the commit.

I'm not sure why the macOS test bench is still failing, although it seems most other PR's are failing the macOS tests, so perhaps it's possible it's an issue with the test bench?

cmarmo · 2020-07-14T12:15:18Z

I'm not sure why the macOS test bench is still failing, although it seems most other PR's are failing the macOS tests, so perhaps it's possible it's an issue with the test bench?

This is an issue on upstream/master, #17913 is meant to fix it.

ForrestCKoch · 2020-07-15T08:02:07Z

Okay, I've merged in the fix and all tests seem to be passing now. Is there anything else I should do here?

jeremiedbb

just a last comment, otherwise lgtm. Thanks @ForrestCKoch !

sklearn/metrics/tests/test_pairwise.py

sklearn/metrics/pairwise.py

thomasjpfan

Otherwise LGTM!

doc/whats_new/v0.24.rst

sklearn/metrics/tests/test_pairwise.py

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

…t-learn#15730) Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com> Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

set dtype=np.double in np.var usage

4feb1dd

ForrestCKoch mentioned this pull request Nov 28, 2019

pairwise_distances fails with metric='seuclidean' if input is not of type np.double #15731

Closed

jnothman reviewed Nov 28, 2019

View reviewed changes

ForrestCKoch added 4 commits November 29, 2019 10:37

add tests

14ebd57

remove trailing whitespace

ca02371

fix formatting for CircleCI

1a531ff

remove concrete datatypes from test -- add test for half-precision fl…

a6dac74

…oats

remove mahalanobis from test

0d885f8

jeremiedbb reviewed Dec 3, 2019

View reviewed changes

ForrestCKoch and others added 9 commits December 5, 2019 13:47

Update sklearn/metrics/tests/test_pairwise.py

3b13ffa

Co-Authored-By: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

as per @jeremiedbb 's suggestsions

e8bdeaa

merging

dc42724

expand on comment

25f6b78

formatting

ebe11a3

add what's new entry

3c6f99b

remove trailing whitespace

06b03bd

remove edit temporarily

60fbd05

merge changes from scikit-learn master branch

5f5651f

jeremiedbb approved these changes Dec 5, 2019

View reviewed changes

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

Update sklearn/metrics/tests/test_pairwise.py

c90412f

Co-Authored-By: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

jnothman reviewed Dec 8, 2019

View reviewed changes

ForrestCKoch added 4 commits December 11, 2019 16:13

add all numeric metrics from scipy -- pushing to see which ones need …

082a647

…to be xfail

merge changes from scikit-learn master branch

1c72396

trying to pass linting

c939a33

trying to pass linting

48e9452

ForrestCKoch added 10 commits July 14, 2020 15:57

adjust tolerence for assert_allclose when dtype==np.float32 && metric…

371feb1

…=='euclidean'

correct typo too->to

becdd36

making the linter happy

ccb0ddd

linting ...

a1096ea

apply reduced tolerence to all np.float32 tests

31f762a

remove trailing whitespace

0a27ac1

precompute parameters to avoid warnings

cfe53e3

linting ...

bc820a4

fix logic mistake, and try only for cosine again

68a5638

remove excess empty line

0caf522

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn

4437ffc

jeremiedbb approved these changes Jul 15, 2020

View reviewed changes

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

change tolerence for all float32 tests

27075ca

thomasjpfan reviewed Jul 15, 2020

View reviewed changes

sklearn/metrics/pairwise.py Outdated Show resolved Hide resolved

ForrestCKoch added 2 commits July 16, 2020 11:26

apply fix only for scipy < 1.5

3acc8fe

remove trailing whitespace

6ffe8ba

thomasjpfan approved these changes Jul 16, 2020

View reviewed changes

doc/whats_new/v0.24.rst 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

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

ForrestCKoch and others added 4 commits July 17, 2020 11:51

Update sklearn/metrics/tests/test_pairwise.py

d6bc709

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Update sklearn/metrics/tests/test_pairwise.py

e0f46d8

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Update doc/whats_new/v0.24.rst

ac16bc4

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Update sklearn/metrics/tests/test_pairwise.py

3120a11

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

thomasjpfan changed the title ~~[Mrg] set dtype=np.double in np.var usage~~ FIX Sets dtype for pairwise_distances when using metric='seuclidean' Jul 17, 2020

thomasjpfan changed the title ~~FIX Sets dtype for pairwise_distances when using metric='seuclidean'~~ FIX Sets dtype for pairwise_distances when metric='seuclidean' Jul 17, 2020

thomasjpfan merged commit b4678ca into scikit-learn:master Jul 17, 2020

ogrisel added this to the 0.23.2 milestone Aug 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Sets dtype for pairwise_distances when metric='seuclidean' #15730

FIX Sets dtype for pairwise_distances when metric='seuclidean' #15730

ForrestCKoch commented Nov 28, 2019 •

edited

Loading

jnothman left a comment

jeremiedbb commented Nov 28, 2019

ForrestCKoch commented Nov 29, 2019 •

edited

Loading

jeremiedbb commented Dec 3, 2019

jeremiedbb left a comment

jeremiedbb Dec 3, 2019

ForrestCKoch Dec 5, 2019 •

edited

Loading

jeremiedbb Dec 3, 2019

ForrestCKoch Dec 5, 2019

jeremiedbb commented Dec 4, 2019

ForrestCKoch commented Dec 5, 2019 •

edited

Loading

jeremiedbb left a comment

jnothman Dec 5, 2019

ForrestCKoch Dec 9, 2019

jnothman Dec 9, 2019

ForrestCKoch Dec 11, 2019

ForrestCKoch commented Jul 14, 2020

cmarmo commented Jul 14, 2020

ForrestCKoch commented Jul 15, 2020

jeremiedbb left a comment

thomasjpfan left a comment

		# check that pairwise distances gives the same result as pdist and cdist
		# regardless of input datatype

FIX Sets dtype for pairwise_distances when metric='seuclidean' #15730

FIX Sets dtype for pairwise_distances when metric='seuclidean' #15730

Conversation

ForrestCKoch commented Nov 28, 2019 • edited Loading

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

jnothman left a comment

Choose a reason for hiding this comment

jeremiedbb commented Nov 28, 2019

ForrestCKoch commented Nov 29, 2019 • edited Loading

jeremiedbb commented Dec 3, 2019

jeremiedbb left a comment

Choose a reason for hiding this comment

jeremiedbb Dec 3, 2019

Choose a reason for hiding this comment

ForrestCKoch Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

jeremiedbb Dec 3, 2019

Choose a reason for hiding this comment

ForrestCKoch Dec 5, 2019

Choose a reason for hiding this comment

jeremiedbb commented Dec 4, 2019

ForrestCKoch commented Dec 5, 2019 • edited Loading

jeremiedbb left a comment

Choose a reason for hiding this comment

jnothman Dec 5, 2019

Choose a reason for hiding this comment

ForrestCKoch Dec 9, 2019

Choose a reason for hiding this comment

jnothman Dec 9, 2019

Choose a reason for hiding this comment

ForrestCKoch Dec 11, 2019

Choose a reason for hiding this comment

ForrestCKoch commented Jul 14, 2020

cmarmo commented Jul 14, 2020

ForrestCKoch commented Jul 15, 2020

jeremiedbb left a comment

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

ForrestCKoch commented Nov 28, 2019 •

edited

Loading

ForrestCKoch commented Nov 29, 2019 •

edited

Loading

ForrestCKoch Dec 5, 2019 •

edited

Loading

ForrestCKoch commented Dec 5, 2019 •

edited

Loading