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

WIP: Constrain longitude between -180 and 180 #93

Merged
merged 6 commits into from Aug 10, 2018

Conversation

Projects
None yet
5 participants
@zachasme
Copy link
Contributor

zachasme commented Jul 17, 2018

The longitude range [0,360] tripped me up. Are you still interested in pushing the [-180,180] range down to C code, cf. this comment?

If so I will go over the test suite as well.

@nrabinowitz nrabinowitz requested review from isaacbrodsky and dfellis Jul 19, 2018

@nrabinowitz

This comment has been minimized.

Copy link
Contributor

nrabinowitz commented Jul 19, 2018

I don't object, but technically this might need to be a major version change, and as such might be worth putting off. Though in practice it wouldn't actually break the Java or JS bindings, it would just render their normalization functions unnecessary. Would like thoughts from @isaacbrodsky and @dfellis.

@nrabinowitz

This comment has been minimized.

Copy link
Contributor

nrabinowitz commented Jul 19, 2018

(I should have added, thanks for the PR! I didn't actually realize this change could be implemented in such a minimal fashion.)

@dfellis

This comment has been minimized.

Copy link
Collaborator

dfellis commented Jul 19, 2018

I'm really excited for this change, and also surprised that nothing needed to be done to the handling of latitudes, which I thought were similarly in the [0, Pi) range instead of [-Pi/2, Pi/2) range. (But then looking at the rest of the function it assumes that the north pole is Pi/2 and the south pole at -Pi/2.)

Since it still accepts inputs greater than Pi for latitude and then converts them to the expected range, I don't think this is a backwards incompatible change (since the greater than Pi radians are equivalent to the less than 0 radians on the globe), especially for those using the bindings where it will run it through the constrain functions and convert to degrees from radians.

My only concerns are whether or not other parts of the code have hardwired expectations of only positive radians. The test suite shows that geoToH3 is perfectly fine, which is great and was my biggest worry, but the failures in testH3SetToLinkedGeo and testH3SetToVertexGraph imply that there may be positive-only expectations hardwired in those features and the failure in testH3Api implies that the constrainLng call might be breaking NAN and/or INFINITY detection on the inputs.

@nrabinowitz

This comment has been minimized.

Copy link
Contributor

nrabinowitz commented Jul 19, 2018

The test failures for testH3SetToLinkedGeo and testH3SetToVertexGraph are segfaults, which suggests they might be fixed by #94 - it would be worth rebasing to master and seeing if those issues go away.

@isaacbrodsky

This comment has been minimized.

Copy link
Collaborator

isaacbrodsky commented Jul 19, 2018

@zachasme thanks for the PR, I think it definitely makes sense to move this into the core rather than the bindings.

You may want to rebase now that #94 is merged, since that improves support for negative coordinates. testH3SetToVertexGraph gets fixed, but it looks like testH3SetToLinkedGeo still has some failure have rebasing. I didn't look into that yet. testH3Api looks good to me, it's failing on a test for exact vertices, not the Infinity/NaN handling.

I don't think this requires a major version bump. Input should not be impacted and supported both negative and positive coordinates before. Output is different but is equivalent, and bindings were doing this transform before anyways.

@nrabinowitz

This comment has been minimized.

Copy link
Contributor

nrabinowitz commented Jul 25, 2018

Hi @zachasme - I think we're all on board for this change. Are you planning to do the test updates, or should one of the maintainers take this on?

@zachasme

This comment has been minimized.

Copy link
Contributor

zachasme commented Jul 26, 2018

I'm planning to finish this when I get back from vacation next week.

@zachasme zachasme force-pushed the zachasme:latlon-range-change branch from 1364e4e to 5c55f79 Aug 6, 2018

@zachasme

This comment has been minimized.

Copy link
Contributor

zachasme commented Aug 6, 2018

The failure in testH3SetToLinkedGeo is due to the loops being reversed, possibly related to #53?

I'm updating the test inputfiles using

find tests/inputfiles -name 'bc*centers.txt' -print -exec awk -i inplace '{if(180<$3) printf "%s %s %.6f\n", $1, $2, $3-360; else printf "%s %s %.6f\n", $1, $2, $3}' {} \;
find tests/inputfiles -name 'res*ic.txt' -print -exec awk -i inplace '{if(180<$3) printf "%s %s %.10f\n", $1, $2, $3-360; else printf "%s %s %.10f\n", $1, $2, $3}' {} \;
find tests/inputfiles -name '*cells.txt'   -print -exec awk -i inplace 'NF==1{print;next} {if(180<$2) printf "   %s %.9f\n", $1, $2-360; else printf "   %s %.9f\n", $1, $2}' {} \;

which fixes many tests, but it appears some hexes still produce longitudes outside the range. How did you originally produce your test inputfiles?

@nrabinowitz

This comment has been minimized.

Copy link
Contributor

nrabinowitz commented Aug 6, 2018

The failure in testH3SetToLinkedGeo is due to the loops being reversed, possibly related to #53?

Hm. I will look into this today.

  • Changes in the order of the list of loops are fine (we will normalize in #53, but even after that the order of the holes is undefined), and can be addressed in tests
  • Changes in the starting coordinate of a given loop sequence are fine (all loops are closed, so the start coord is undefined), and can be addressed in tests
  • Changes in the order or direction (clockwise vs counter-clockwise) of a given loop sequence is not fine, and will need to be addressed in code
@nrabinowitz

This comment has been minimized.

Copy link
Contributor

nrabinowitz commented Aug 7, 2018

Re test failures in testH3SetToLinkedGeo - this is okay; the order of the loops within the list of loops is currently undefined/arbitrary, so reversing that order is acceptable.

Patch for this:

--- a/src/apps/testapps/testH3SetToLinkedGeo.c
+++ b/src/apps/testapps/testH3SetToLinkedGeo.c
@@ -119,10 +119,12 @@ TEST(hole) {
     H3_EXPORT(h3SetToLinkedGeo)(set, numHexes, &polygon);
 
     t_assert(countLinkedLoops(&polygon) == 2, "2 loops added to polygon");
-    t_assert(countLinkedCoords(polygon.first) == 6 * 3,
-             "All outer coords added to first loop");
-    t_assert(countLinkedCoords(polygon.first->next) == 6,
-             "All inner coords added to second loop");
+    // Note: This isn't strictly correct, and should be reversed when
+    // https://github.com/uber/h3/issues/53 is resolved
+    t_assert(countLinkedCoords(polygon.first) == 6,
+             "All inner coords added to first loop");
+    t_assert(countLinkedCoords(polygon.first->next) == 6 * 3,
+             "All outer coords added to second loop");
 
     H3_EXPORT(destroyLinkedPolygon)(&polygon);
@zachasme

This comment has been minimized.

Copy link
Contributor

zachasme commented Aug 8, 2018

I have applied the patch from @nrabinowitz, and fixed the remaining tests as well.

@nrabinowitz
Copy link
Contributor

nrabinowitz left a comment

LGTM - thanks for putting this together!

@isaacbrodsky
Copy link
Collaborator

isaacbrodsky left a comment

When I looked at the test files I was initially a little surprised rand05centers.txt wasn't updated, but it turned out the geoToH3 test didn't need any update.

@@ -127,6 +127,9 @@ double constrainLng(double lng) {
while (lng > M_PI) {

This comment has been minimized.

@isaacbrodsky

isaacbrodsky Aug 10, 2018

Collaborator

Might be nice to replace this with something constant time in the future

@isaacbrodsky isaacbrodsky merged commit f9be0f9 into uber:master Aug 10, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

isaacbrodsky added a commit to isaacbrodsky/h3 that referenced this pull request Aug 10, 2018

isaacbrodsky added a commit that referenced this pull request Aug 15, 2018

Merge pull request #118 from isaacbrodsky/lon-range-readme
Update examples and docs for longitude range change (#93)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment