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

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

You could add a test here :)

@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 ?

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

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

@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.

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

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.

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

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

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

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

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

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.

You meant scipy right? :p

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

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

You should add the metric to test_neighbors_metrics in test_neighbors.py

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

restarted.

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

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!

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

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).

Ok we should check for jaccard.

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

I think I need to do some benchmark.

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

Hi all, How can I finish this?

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

Closing due to being superseded by #12568