[MRG + 1] Implement haversine metric in pairwise #4458

xuewei4d · 2015-03-28T19:10:53Z

Fixes #4453 #4452 , add haversine distrance for pairwise_distances and document it.

~~I did not find any test for pairwise.py, so I did not add any test cases.~~

landscape-bot · 2015-03-28T19:19:11Z

Code quality remained the same when pulling 3dba895 on xuewei4d:haversine_pairwise into 0117fb5 on scikit-learn:master.

landscape-bot · 2015-03-28T19:28:54Z

Code quality remained the same when pulling c9a02bd on xuewei4d:haversine_pairwise into 0117fb5 on scikit-learn:master.

raghavrv · 2015-03-28T19:50:34Z

You could add a test here :)

raghavrv · 2015-03-28T19:52:33Z

sklearn/metrics/pairwise.py

@@ -426,6 +426,57 @@ def pairwise_distances_argmin(X, Y, axis=1, metric="euclidean",
                                         metric_kwargs)[0]


+def haversine_distances(X, Y=None, ):


(X, Y=None, ) --> (X, y=None)

Ah sorry keep the y uppercased...

(X, Y=None, ) --> (X, Y=None)

xuewei4d · 2015-03-28T19:56:27Z

@ragv I looked into wrong test folders :(

Besides, the testing in test_neighbors.py failed in line 815.

        nbrs = cls(algorithm='ball_tree', metric='haversine')
        assert_raises(ValueError,
                      nbrs.predict,
                      X)
        assert_raises(ValueError,
                      ignore_warnings(nbrs.fit),
                      Xsparse, y)

Is there any reason that haversine metric cannot be used with ball_tree? @jakevdp ?

raghavrv · 2015-03-28T19:56:30Z

sklearn/metrics/pairwise.py

+        with shape (n_samples_X, 2).
+
+    Y : array_like
+        with shape (n_samples_Y, 2).


shape information can go in the first line itself... More description, if available, can follow next :) Like here.

raghavrv · 2015-03-28T20:02:41Z

Besides, the testing in test_neighbors.py failed in line 815.

You haven't validated the data like done here for euclidean_distances. :)

xuewei4d · 2015-03-28T20:20:10Z

@ragv I don't think check_pairwise_arrays will let it pass test_neighbors_badargs in neighbors/test_neighbors.py. It looks like nbrs = cls(algorithm='ball_tree', metric='haversine') is a bad argument.

landscape-bot · 2015-03-28T20:24:32Z

Code quality remained the same when pulling f7670d4 on xuewei4d:haversine_pairwise into 0117fb5 on scikit-learn:master.

raghavrv · 2015-03-28T21:11:37Z

I suggested input (X, Y) validation, simply upon seeing bad args in the test title... sorry :p

haversine metric was introduced in this PR only right? It says ValueError was not raised...

I think since previously haversine was not implemented, there was a test to check if ValueError was raised... Now that we have haversine implemented I think you can go ahead and remove L815-L821.

landscape-bot · 2015-03-28T21:19:41Z

Code quality remained the same when pulling 2b329b4 on xuewei4d:haversine_pairwise into 0117fb5 on scikit-learn:master.

xuewei4d · 2015-03-28T21:29:23Z

Thanks for your comments, @ragv. I think we need someone else to take a look at L815

landscape-bot · 2015-03-28T21:29:29Z

Code quality remained the same when pulling 3746d7d on xuewei4d:haversine_pairwise into 0117fb5 on scikit-learn:master.

coveralls · 2015-03-28T21:31:52Z

Coverage decreased (-0.0%) to 95.11% when pulling 3746d7d on xuewei4d:haversine_pairwise into 0117fb5 on scikit-learn:master.

raghavrv · 2015-03-28T21:35:19Z

There is an unrelated test failure in Python 3.4 alone... :-/ Others seem to pass... Probably temporary server unavailability...

xuewei4d · 2015-04-01T12:58:27Z

Any review on this PR @amueller ? Should I need to check the data is in the domain of cos, sin? ~~Spicy~~ Scipy will give a running time warning anyway.

raghavrv · 2015-04-01T16:46:46Z

You meant scipy right? :p

raghavrv · 2015-04-01T16:59:11Z

Also in your PR description change fix to fixes so that github can automatically close that issue when this PR gets merged :)

amueller · 2015-04-01T17:12:30Z

Hey @xuewei4d. Sorry I was sick over the weekend and just catching up. It is fine that BallTree doesn't work with this metric.

amueller · 2015-04-01T17:17:43Z

sklearn/metrics/tests/test_pairwise.py

    # Test with tuples as X and Y
    X_tuples = tuple([tuple([v for v in row]) for row in X])
    Y_tuples = tuple([tuple([v for v in row]) for row in Y])
    S2 = pairwise_distances(X_tuples, Y_tuples, metric="euclidean")
    assert_array_almost_equal(S, S2)
+
+    # Test haversine distnace


amueller · 2015-04-01T17:21:16Z

You should add the metric to test_neighbors_metrics in test_neighbors.py

amueller · 2015-04-01T17:22:31Z

Does the tree also raise an exception if the data is not 2d? Maybe that should be tested in test_neighbors_badargs?

amueller · 2015-04-05T19:44:09Z

restarted.

jakevdp · 2015-05-05T20:44:32Z

Yes, haversine distances are defined only in two dimensions. See https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/neighbors/dist_metrics.pyx#L955

jakevdp · 2015-05-05T20:50:11Z

Just out of curiosity, could you check the performance of this algorithm vs. using the DistanceMetric class directly? e.g.

In [1]: from sklearn.neighbors import DistanceMetric

In [2]: import numpy as np

In [3]: X = np.random.random((100, 2))

In [20]: metric = DistanceMetric.get_metric('haversine')

In [21]: dist = metric.pairwise(X, X)

The difference is that this doesn't do any broadcasting, so it may be faster and/or more memory efficient.

Note that the DistanceMetric stuff is not really documented in the package, as I considered it primarily a workhorse for the BallTree/KDTree. The only reason the Python interface exists is to facilitate unit tests, but there's no reason we can't take advantage of it elsewhere!

amueller · 2015-05-05T21:33:56Z

I wasn't really aware of the DistanceMetric interface. Shame on me :-/ could we also use that for the jaccard?

jakevdp · 2015-05-05T21:39:14Z

Yes, it would work for Jaccard! Actually, it will work for any of the metrics supported by BallTree. For pairwise distances, it tends to be a factor of a few slower than the equivalent scipy routines, though (if I remember correctly; I did the benchmarks a long time ago).

amueller · 2015-05-05T21:40:01Z

Ok we should check for jaccard.

amueller · 2015-06-15T19:11:50Z

what was the status here? Just waiting for another review?

xuewei4d · 2015-06-16T18:35:59Z

I think I need to do some benchmark.

raghavrv · 2016-11-27T03:55:21Z

@xuewei4d Would you have some time to finish this up?

ibrahimsharaf · 2017-08-23T11:54:04Z

Hi all, How can I finish this?

jnothman · 2017-08-23T21:31:51Z

Could you please benchmark how fast this is compared to an implementation based on Distance metric as suggested by jakevdp above?

jnothman · 2019-04-16T04:00:53Z

Closing due to being superseded by #12568

xuewei4d force-pushed the haversine_pairwise branch from 3dba895 to c9a02bd Compare March 28, 2015 19:24

raghavrv reviewed Mar 28, 2015
View reviewed changes

xuewei4d changed the title ~~[MRG] Implement haversine metric in pairwise~~ [WIP] Implement haversine metric in pairwise Mar 28, 2015

xuewei4d changed the title ~~[WIP] Implement haversine metric in pairwise~~ [MRG] Implement haversine metric in pairwise Mar 28, 2015

amueller reviewed Apr 1, 2015
View reviewed changes

amueller added the Waiting for Reviewer label Dec 10, 2015

raghavrv added Stalled and removed Waiting for Reviewer labels Nov 27, 2016

rth mentioned this pull request Mar 27, 2017

The haversine metric in DBSCAN is too slow #8655

Open

jnothman added this to the 0.20 milestone Jun 14, 2017

jnothman added Easy Well-defined and straightforward way to resolve Need Contributor labels Jun 14, 2017

lesteve added help wanted and removed Need Contributor labels Oct 18, 2017

glemaitre modified the milestones: 0.20, 0.21 Jun 13, 2018

This was referenced Nov 9, 2018

Support Haversine distance in NearestNeighbors #12552

Closed

haversine distance for OPTICS #12480

Open

Implement haversine metric in pairwise, document properly #4453

Closed

This was referenced Nov 16, 2018

ENH Support Haversine distance in pairwise_distances #12568

Merged

Ok get kriging matrix coordinates fix GeoStat-Framework/PyKrige#99

Closed

jnothman removed this from the 0.21 milestone Apr 16, 2019

rth closed this in #12568 Apr 24, 2019

		@@ -426,6 +426,57 @@ def pairwise_distances_argmin(X, Y, axis=1, metric="euclidean",
		metric_kwargs)[0]


		def haversine_distances(X, Y=None, ):

[MRG + 1] Implement haversine metric in pairwise #4458

[MRG + 1] Implement haversine metric in pairwise #4458

Conversation

xuewei4d commented Mar 28, 2015

landscape-bot commented Mar 28, 2015

landscape-bot commented Mar 28, 2015

raghavrv commented Mar 28, 2015

raghavrv Mar 28, 2015

Choose a reason for hiding this comment

raghavrv Mar 28, 2015

Choose a reason for hiding this comment

xuewei4d commented Mar 28, 2015

raghavrv Mar 28, 2015

Choose a reason for hiding this comment

raghavrv commented Mar 28, 2015

xuewei4d commented Mar 28, 2015

landscape-bot commented Mar 28, 2015

raghavrv commented Mar 28, 2015

landscape-bot commented Mar 28, 2015

xuewei4d commented Mar 28, 2015

landscape-bot commented Mar 28, 2015

coveralls commented Mar 28, 2015

raghavrv commented Mar 28, 2015

xuewei4d commented Apr 1, 2015

raghavrv commented Apr 1, 2015

raghavrv commented Apr 1, 2015

amueller commented Apr 1, 2015

amueller Apr 1, 2015

Choose a reason for hiding this comment

amueller commented Apr 1, 2015

amueller commented Apr 1, 2015

amueller commented Apr 5, 2015

jakevdp commented May 5, 2015

jakevdp commented May 5, 2015

amueller commented May 5, 2015

jakevdp commented May 5, 2015

amueller commented May 5, 2015

amueller commented Jun 15, 2015

xuewei4d commented Jun 16, 2015

raghavrv commented Nov 27, 2016 • edited

ibrahimsharaf commented Aug 23, 2017

jnothman commented Aug 23, 2017 • edited

jnothman commented Apr 16, 2019

raghavrv commented Nov 27, 2016 •

edited

jnothman commented Aug 23, 2017 •

edited