WIP: Constrain longitude between -180 and 180 #93

zachasme · 2018-07-17T10:28:47Z

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 · 2018-07-19T16:55:11Z

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 · 2018-07-19T16:55:48Z

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

dfellis · 2018-07-19T17:10:35Z

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 · 2018-07-19T18:43:18Z

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 · 2018-07-19T18:46:23Z

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

8000

nrabinowitz · 2018-07-25T18:03:56Z

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 · 2018-07-26T07:46:14Z

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

zachasme · 2018-08-06T13:19:12Z

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 · 2018-08-06T17:28:14Z

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 Move GeoJSON normalization logic into core C lib #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 · 2018-08-07T18:59:38Z

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

…ToLinkedGeo test

zachasme · 2018-08-08T10:40:36Z

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

nrabinowitz

LGTM - thanks for putting this together!

isaacbrodsky

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.

isaacbrodsky · 2018-08-10T18:10:51Z

src/h3lib/lib/geoCoord.c

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


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

Update examples and docs for longitude range change (#93)

Constrain longitude between -180 and 180

Update examples and docs for longitude range change (uber#93)

nrabinowitz requested review from isaacbrodsky and dfellis July 19, 2018 16:52

ENH: Constrain longitude between -180 and 180

5c55f79

zachasme force-pushed the latlon-range-change branch from 1364e4e to 5c55f79 Compare August 6, 2018 09:40

zachasme added 2 commits August 6, 2018 13:56

FIX: Wrap testH3Api at 180

1079bd5

FIX: Wrap bc*centers.txt testfiles at 180

af13fa3

zachasme added 3 commits August 8, 2018 11:39

FIX: apply patch from nrabinowitz, reverses loop order in failing set…

d397a6a

…ToLinkedGeo test

FIX: Enforce lower bound in constrainLng

8bf7fa6

FIX: Wrap remaining test input longitudes at 180

71682b7

nrabinowitz approved these changes Aug 9, 2018

View reviewed changes

isaachier approved these changes Aug 9, 2018

View reviewed changes

isaacbrodsky approved these changes Aug 10, 2018

View reviewed changes

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

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

Update examples and docs for uber#93

b3330ff

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

Merge pull request #118 from isaacbrodsky/lon-range-readme

23a7fcf

Update examples and docs for longitude range change (#93)

nrabinowitz mentioned this pull request Sep 23, 2019

Improved breadth-first 'spidering' polyfill algorithm #282

Merged

mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022

Merge pull request uber#93 from zachasme/latlon-range-change

86ae4dc

Constrain longitude between -180 and 180

mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022

Update examples and docs for uber#93

64f9335

mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022

Merge pull request uber#118 from isaacbrodsky/lon-range-readme

70751e7

Update examples and docs for longitude range change (uber#93)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Constrain longitude between -180 and 180 #93

WIP: Constrain longitude between -180 and 180 #93

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

WIP: Constrain longitude between -180 and 180 #93

WIP: Constrain longitude between -180 and 180 #93

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment