Closed phayes closed 1 year ago
Do you know how this is being tested within the upstream library? Is there a test suite we could lift or seek inspiration from?
@michaelkirk, I've gone looking for a test suite, and couldn't see anything obvious. I'll keep looking.
At the very least I want to test it against a bunch of weird polygons and ensure that it has the same output as upstream.
Digging a bit, I haven't found a clear cpp test suite either, but I found some threads to chase down:
There is this cli tool, which I think is being used in a test harness for the cpp program's polygon area calculation: https://github.com/geographiclib/geographiclib/blob/main/tools/Planimeter.cpp
A keyword to search for in the code base is "Planimeter" - I found a series of relevant looking tests in other languages: https://github.com/geographiclib/geographiclib-octave/blob/main/inst/geographiclib_test.m#L644
Hi @michaelkirk ,
This is now ready for review. I've added copies of all the tests I could find, and they are all now passing.
Changes since the initial PR:
Winding
enum to make sure we are properly handling both clockwise and counterclockwise windingsaccurate
is now enabled by default. My thinking here is that if someone is making use of a geodesic area calculator, they're already motivated to have good accuracy over raw speed, so we should enable it by default. Marked it as draft again as I found a bug in the add_edge
method. Fixing...
The bug in add_edge
is now fixed. Ready for review!
Hi @michaelkirk,
I've addressed the comments, the following decisions are outstanding:
Do we report the perimeter for degenerate polygons to be NaN
or to be whatever naturally falls out of the algorithim? NOTE: This is now fixed and matches upstream.
Winding. I really like the current pattern in the PR of setting the winding when creating the PolygonArea
, it feels like a more natural API. You define the winding, then you add points according to that winding. However, feel free to push back on this.
transitdirect
. My previous understanding was wrong. Working on a fix and more tests now. NOTE: This is now fixed and matches upsteam.
Hi @michaelkirk ,
I believe I fixed all the outstanding issues. transitdirect
and degenerate polygons now work and behavior matches upstream. The issue ended up being related to the mask being used. I was using the default mask, but PolygonArea
needs to be using a specific mask. This is now fixed and everything has fallen into place.
The only outstanding decision is that winding is specified during creation of the PolygonArea
. Just need confirmation that we like this ergonomically.
bors r+
Thanks so much!
Tests are failing with what looks to be a very small discrepancy. I'm comfortable merging it as is if you want to just bump the tolerance and merge yourself.
bors d=phayes
:v: phayes can now approve this pull request. To approve and merge a pull request, simply reply with bors r+
. More detailed instructions are available here.
That's super odd, the tests pass on my machine. I wonder if there's subtle floating-point differences on Arm64 vs whatever is running on GitHub.
bors try
bors r+
Build succeeded:
[X] I added an entry to
CHANGES.md
if knowledge of this change could be valuable to users.This PR adds
PolygonArea
for calculating the perimeter and area of a polygon on a Geodesic. It is a port of the PolygonArea class in Geographic Lib (https://github.com/geographiclib/geographiclib/blob/main/src/PolygonArea.cpp).This PR also adds
accurate
as an optional dependency (enabled by default), that allows for very accurate summations. This replaces what would have gone inaccumulator.rs