Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add haversine distance calc example #22

Merged
merged 2 commits into from Jan 29, 2018
Merged

Conversation

jogly
Copy link
Contributor

@jogly jogly commented Jan 29, 2018

adds an example for referencing in issue #18

@CLAassistant
Copy link

CLAassistant commented Jan 29, 2018

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Jan 29, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling c81c042 on joegilley:jg-distance-example into 9308e68 on uber:master.

adds an example for referencing in issue uber#18
#include <math.h>
#include <stdio.h>

double dist(double th1, double ph1, double th2, double ph2) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is an example, maybe include a comment on the algo here with a link to some reference?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, would it be better to just expose _geoDistRads or _geoDistKm from geoCoord?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could do! i expect cutting out the middle step and creating h3DistKm would be more valuable and appropriate for this lib, since that's what I'm doing here anyway, but cross that bridge if/when we find the need?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe include a comment on the algo here

done!

@@ -0,0 +1,61 @@
/*
* Copyright 2017 Uber Technologies, Inc.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2018

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it 2018 already?!

#include <math.h>
#include <stdio.h>

double dist(double th1, double ph1, double th2, double ph2) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment for this function? What are the parameters, is it Haversine distance, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

Copy link
Collaborator

@isaacbrodsky isaacbrodsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me; @nrabinowitz raises a good point about publicizing geoDistRads/geoDistKm, but I think this is fine considering the haversine formula seems to be well documented online.

// 555 Market St @ resolution 15
H3Index h3HQ2 = stringToH3("8f283082a30e623");

double distance = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like double distance is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uh oh, possible to configure linter to catch? compiling with -Wall catches this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

publicizing geoDistRads/geoDistKm

I suggest we put out a minor version feature bump with these two and h3DistRads/h3DistKm; doing that is out of scope for this addition. like I mentioned to @nrabinowitz , I expect the h3 distance functions will be more useful and appropriate than yet another set of geo distance functions.

@dfellis
Copy link
Collaborator

dfellis commented Jan 29, 2018

Making geoDistKm a first-class citizen really does sound like a useful change (and worthy of 3.1.0). You can add my vote on that change, too. :)

@jogly
Copy link
Contributor Author

jogly commented Jan 29, 2018

created #23 for tracking

Copy link
Collaborator

@nrabinowitz nrabinowitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for adding the issue to track the feature addition.

@jogly jogly merged commit f53dc76 into uber:master Jan 29, 2018
@jogly jogly deleted the jg-distance-example branch January 29, 2018 19:11
mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants