Adding support geodesic area and perimeter calculations from geographlib-rs #988

phayes · 2023-02-27T20:49:10Z

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This is a PR for adding a GeodesicArea trait for highly accurate geodesic area and perimeter calculations.

See georust/geographiclib-rs#49

…iclib-rs

geo/src/algorithm/geodesic_area.rs

Co-authored-by: Corey Farwell <coreyf@rwell.org>

phayes · 2023-03-02T16:10:37Z

This is now ready for merging / review.

An open question is whether we like what I've done with having a bunch of different methods for getting area, perimeter or both. Maybe we want to ignore perimeter and only focus on area? Maybe we always want to get both together since they're computed together?

michaelkirk

Thanks for another great PR!

The overall direction I feel good about, but I had some questions/comments.

michaelkirk · 2023-03-02T20:09:51Z

geo/src/algorithm/geodesic_area.rs

+    }
+}
+
+fn geodesic_area(poly: &Polygon, sign: bool, reverse: bool, exterior_only: bool) -> (f64, f64) {


reverse and exterior_only seems to always be false. Do you want to get rid of them? Or if you want to keep them, can you comment as to their future purpose?

Originally I was planning on more features, but decided to reign it in a bit. So I guess some open questions:

Do we want to fully support clockwise-wounded geometries? The Documentation (https://docs.rs/geo/latest/geo/#semantics) states "The geospatial types provided here aim to adhere to the OpenGIS Simple feature access standards." This suggests to me that we don't really need to actually support reverse-wound geometries other than on a minimal "best effort" basis. If this is true, I'll remove the "reverse" option here to only fully support counterclockwise-wound geometries as per the Simple Feature spec.

Is it useful to have an option to only get the perimeter of the exterior? Originally I thought this might be useful, but in retrospect just calling geodesic_length() on the exterior ring does the same thing. The only trickly thing here is that sometimes I've seen polygons constructed where the first point of the exterior ring and the last point of the exterior ring are not the same (there's an implicit closure of the ring, instead of an explicit closure by duplicating the origin point). If this case calling geodesic_length() on the exterior-ring would result in an incorrect result, but we could correct for that here knowing that the exterior-rings and interior-rings should always close, even if only implicitly. Do we want to support these non-compliant polygons here, or just leave it to exterior().geodesic_length() ?

geo/src/algorithm/geodesic_area.rs

phayes · 2023-03-03T16:42:26Z

I've added a bunch more tests to confirm that my "winding correction logic" is good and adjusted the epsilons. I would appreciate another test run on your local to confirm that my epsilons are good (I still think it's really weird that there's a difference in behavior between our machines!).

michaelkirk · 2023-03-03T17:09:28Z

I've added a bunch more tests to confirm that my "winding correction logic" is good and adjusted the epsilons.

Without the "winding correction logic" you added, the mis-wound holes would add to rather than subtract from the area, right?

That logic seems like it would be helpful for most people, and the tests give me confidence that you've implemented it correctly.

As devil's advocate, I'm trying to imagine if there's a world where people would intentionally wind their polygons this way, where the existing computation would be meaningful, and now we've inadvertently broken some special use case that these people might have... but I'm having a hard time imagining such a thing.

On balance, I think what you've done makes sense, and is a nice guard rail, so we should merge it.

I would appreciate another test run on your local to confirm that my epsilons are good

Passing now — Thanks!

(I still think it's really weird that there's a difference in behavior between our machines!).

I'm no expert on floating point implementations, but it's my understanding that there are small discrepancies across architectures and compilers. Some operations are encompassed by the IEEE754 spec, and should perform uniformly, but some operations (e.g. trigonometric functions) are not and might diverge.

I don't know if that's what's going on here, but it's common enough that if the divergence is very small, I've chosen to add a small epsilon for testing and spend my time elsewhere.

Overall, this LGTM, but I'll wait a couple days before merging in case anyone else wants to chime in.

(Though I think it needs a cargo fmt before we can merge)

bors try

bors · 2023-03-03T17:14:54Z

try

Build failed:

ci result

phayes · 2023-03-03T18:01:20Z

Without the "winding correction logic" you added, the mis-wound holes would add to rather than subtract from the area, right?

Correct

As devil's advocate, I'm trying to imagine if there's a world where people would intentionally wind their polygons this way, where the existing computation would be meaningful, and now we've inadvertently broken some special use case that these people might have... but I'm having a hard time imagining such a thing.

There is prior art for always considering the interior rings to reduce the area. I've been using QGIS to double-check my numbers in my tests, and QGIS always calculates interior-rings as reducing the magnitude of the area. So what we have here matches QGIS at the very least.

On balance, I think what you've done makes sense, and is a nice guard rail, so we should merge it.

Yay!

I've now run cargo fmt

bors try

bors · 2023-03-03T18:01:22Z

🔒 Permission denied

Existing reviewers: click here to make phayes a reviewer

michaelkirk · 2023-03-03T18:03:39Z

bors try

bors · 2023-03-03T18:09:29Z

try

Build succeeded:

ci result

frewsxcv

lgtm! thank you for adding this 🌻

will leave this open in case michael kirk or someone else has comments

geo/src/algorithm/geodesic_area.rs

urschrei · 2023-03-05T16:59:34Z

I think the area calculation you've got now is the correct approach – I don't think we have to cater to what would be a fundamental divergence from the spec and convention on the part of a user.

Co-authored-by: Corey Farwell <coreyf@rwell.org>

michaelkirk · 2023-03-06T18:45:15Z

bors r+

bors · 2023-03-06T18:51:22Z

Build succeeded:

ci result

phayes added 2 commits February 27, 2023 12:45

Adding support geodesic area and perimeter calculations from geograph…

6af9e4a

…iclib-rs

Updating changes to report new GeodesicArea trait

38cd43a

phayes marked this pull request as draft February 27, 2023 20:49

Adding more asserts to tests

cd74de1

frewsxcv reviewed Feb 28, 2023

View reviewed changes

geo/src/algorithm/geodesic_area.rs Outdated Show resolved Hide resolved

Update geo/src/algorithm/geodesic_area.rs

e6eea06

Co-authored-by: Corey Farwell <coreyf@rwell.org>

michaelkirk self-requested a review March 1, 2023 03:14

phayes added 2 commits March 1, 2023 18:47

Bumping geographiclib version

41c6c21

Fixing merge conflict

94ab281

phayes marked this pull request as ready for review March 2, 2023 16:09

michaelkirk reviewed Mar 2, 2023

View reviewed changes

Adding more tests, adjusting epsilons for test asserts

aea403e

Adding another test for very large polygons

52162d0

bors bot added a commit that referenced this pull request Mar 3, 2023

Try #988:

b472563

Cargo fmt

adaaa7f

bors bot added a commit that referenced this pull request Mar 3, 2023

Try #988:

201dbd6

frewsxcv approved these changes Mar 5, 2023

View reviewed changes

geo/src/algorithm/geodesic_area.rs Outdated Show resolved Hide resolved

urschrei approved these changes Mar 5, 2023

View reviewed changes

phayes and others added 2 commits March 5, 2023 09:52

Update geo/src/algorithm/geodesic_area.rs

03e23dd

Co-authored-by: Corey Farwell <coreyf@rwell.org>

Removing unused generic param

0d15e56

bors bot merged commit bc16e13 into georust:main Mar 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support geodesic area and perimeter calculations from geographlib-rs #988

Adding support geodesic area and perimeter calculations from geographlib-rs #988

phayes commented Feb 27, 2023 •

edited

Loading

phayes commented Mar 2, 2023 •

edited

Loading

michaelkirk left a comment

michaelkirk Mar 2, 2023

phayes Mar 2, 2023 •

edited

Loading

phayes commented Mar 3, 2023

michaelkirk commented Mar 3, 2023

bors bot commented Mar 3, 2023

phayes commented Mar 3, 2023 •

edited

Loading

bors bot commented Mar 3, 2023

michaelkirk commented Mar 3, 2023

bors bot commented Mar 3, 2023

frewsxcv left a comment

urschrei commented Mar 5, 2023

michaelkirk commented Mar 6, 2023

bors bot commented Mar 6, 2023

Adding support geodesic area and perimeter calculations from geographlib-rs #988

Adding support geodesic area and perimeter calculations from geographlib-rs #988

Conversation

phayes commented Feb 27, 2023 • edited Loading

phayes commented Mar 2, 2023 • edited Loading

michaelkirk left a comment

Choose a reason for hiding this comment

michaelkirk Mar 2, 2023

Choose a reason for hiding this comment

phayes Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

phayes commented Mar 3, 2023

michaelkirk commented Mar 3, 2023

bors bot commented Mar 3, 2023

try

phayes commented Mar 3, 2023 • edited Loading

bors bot commented Mar 3, 2023

michaelkirk commented Mar 3, 2023

bors bot commented Mar 3, 2023

try

frewsxcv left a comment

Choose a reason for hiding this comment

urschrei commented Mar 5, 2023

michaelkirk commented Mar 6, 2023

bors bot commented Mar 6, 2023

phayes commented Feb 27, 2023 •

edited

Loading

phayes commented Mar 2, 2023 •

edited

Loading

phayes Mar 2, 2023 •

edited

Loading

phayes commented Mar 3, 2023 •

edited

Loading