turf-area: bug fix: off-by-one bug in area calculation

[Abdumbo99]

• [Y] bug fix
• [Y] Have read and followed the steps for preparing a pull request.

First of all, I would like to thank the contributors behind Mapbox and all of its subprojects for their amazing work. I want to point out a small mistake that I found while re-writing some of the geo calculations for a library in GO, using Mapbox, amongst other projects, as a reference.
As per The GeoJSON Specification (RFC 7946). A Polygon consists of Linear Rings and a linear ring "... is a closed LineString with four or more positions", the reason for the four positions is that "The first and last positions are equivalent, and they MUST contain identical values; their representation SHOULD also be identical", i.e. to ensure the linear ring is closed the first and last position must be identical, deeming the first, or the last for that matter, position redundant and opens the door for some miscalculations similar to the one that I am attempting to fix in this PR.

The "-1" added in this PR solved two problems one is much simpler than the other but it is the main focus of this PR. The issues are:

1. The if (coordsLength <= 2) return 0 in line 102 does not comply with the four positions requirement mentioned above, so a polygon with the invalid coordinates below would pass it, even though it is not a proper polygon (has less than four coordinates). [[[46.78401786031469,24.672371366746333 ], [46.78401786031469, 24.69016675081427 ], [46.78401786031469, 24.672371366746333]]]

2. The second issue is in the calculations. The code cites the source * Reference: Robert. G. Chamberlain and William H. Duquette, "Some Algorithms for Polygons on a Sphere", JPL Publication 07-03, Jet propulsion Laboratory, Pasadena, CA, June 2007 http://trs-new.jpl.nasa.gov/dspace/handle/2014/40409 then uses it properly, almost, at least. From this source we can see the calculation of the area:
   "Assume we are dealing with a polygon with N vertices", after some work the equation becomes:

In this equation we have the same conditions as Geojson's polygons; N and 0 are identical. Thus the equation stops at N-1, i.e. the point just before the duplicate 0th point.
In the code, however, this is not the case, as it stops at the Nth point instead. This extra iteration on the Nth point results in an inconsistent behavior. For example, if we use this multipolygon:
{"type":"MultiPolygon","coordinates":[[[[28.321755510202507,16.35627490376781],[20.424575867090823,1.7575215418945476],[48.254218513706036,20.42650462625916],[36.310934132380964,14.226760576846956],[28.321755510202507,16.35627490376781]]]]}
We will get the good and rather accurate approximated area of: 1139359621777.495, but if we rotate the coordinates upwards, while maintaining the order and right-hand rule we will get the following calculations: 139359621777.4954, 1139359621777.495, 1139359621777.495, and 1139359621777.495. The coordinates of each rotation are respectively:
[ [ 20.424575867090823, 1.7575215418945476 ], [ 48.254218513706036, 20.42650462625916 ], [ 36.310934132380964, 14.226760576846956 ], [ 28.321755510202507, 16.35627490376781 ], [ 20.424575867090823, 1.7575215418945476 ] ]

[[ 48.254218513706036, 20.42650462625916 ], [ 36.310934132380964, 14.226760576846956 ], [ 28.321755510202507, 16.35627490376781 ], [ 20.424575867090823, 1.7575215418945476 ], [ 48.254218513706036, 20.42650462625916 ]]

[[ 36.310934132380964, 14.226760576846956 ], [28.321755510202507, 16.35627490376781 ], [ 20.424575867090823, 1.7575215418945476 ], [ 48.254218513706036, 20.42650462625916 ], [ 36.310934132380964, 14.226760576846956 ]]

[[ 28.321755510202507, 16.35627490376781 ], [ 20.424575867090823, 1.7575215418945476 ], [ 48.254218513706036, 20.42650462625916 ], [36.310934132380964, 14.226760576846956 ], [ 28.321755510202507, 16.35627490376781 ]]

Now while the difference in the calculations is trivial, the inconsistency is, definitely, problematic and is caused by the the extra iteration on the Nth point.
I hope my points are clear, I am ready to clarify any unclear points if needed.
Since this concerns last, first and second point at some point in the calculations I am suspicious that this possibly Fixes (edit:NOT) #1987
NOTE: similar PR can be found here: mapbox/geojson-area#13

[Abdumbo99]

Just a quick note that this does not test against #1987 because it was not in response to it, but the issue could be resolved as a side effect of this PR.

[twelch]

Thank you @Abdumbo99 for submitting this pull request and the background. To help get this accepted, would you be willing to add a unit test demonstrating the issue and that it's fixed?

[Abdumbo99]

Thank you @twelch for the reply, I committed my tests. If there are any style issues in the changes please let me know (I am not fluent in TS).
The tests fail on const coordsLength = coords.length; but pass on const coordsLength = coords.length - 1;
Test "turf-area-length-check" checks for the length properly, i.e. if it is shorter than 4 positions, now the code does not pass a polygon with 3 positions anymore. Now keep in mind that the constructor does indeed prohibit the creation of polygons that contain less than 4 positions, however, the area function double-checks for this criterion, and if the coordinates member variable was modified after creation (as done in the test) it can break the rule and the length check in the area function will pass polygons that it should not pass.
The old code would fail on this test, returning an actual calculation instead of returning 0. The new code passes this test.

Test "turf-area-rotation-consistency" checks for consistency in the calculation of the area if the polygon positions were to be rotated, in a way that does not violate the right-hand rule, i.e. counter-clockwise.
The old code would fail on the very first iteration of the rotation, while it passes on the others, the logs message is included below:

# turf-area-rotation-consistency
not ok 3 should be strictly equal
  ---
    operator: equal
    expected: 1139359621777.4954
    actual:   1139359621777.495

The new code passes this test.

[Abdumbo99]

Also as an extra note this PR does not resolve #1987

[twelch]

Nice work adding the tests @Abdumbo99, very thorough with the rotation. I have one request to improve how you create the invalid polygon and then I'll be happy to approve and merge. Let me know if you run into any issue.

[twelch]

I would suggest creating an invalid Polygon geojson object directly if the helper is not allowing you to.

import { Polygon } from 'geojson'
const invalidPoly: Polygon = {
  type: "Polygon",
  coordinates: [
    [
      [101.0, 0.0],
      [101.0, 0.5],
      [101.5, 0.5],
    ],
  ]
};

[twelch]

Not required, but if these test inputs are only used in a single test, consider moving the definitions down into the test they are used, so that the tests are more self-contained. I think it makes the tests easier to read.

[Abdumbo99]

Alright, I did as you suggested, please let me know if there are any other issues.

[twelch]

@Abdumbo99 it looks like your area function fix is causing one of the test outputs in turf-isobands to change and the continuous integration testing is now failing. If you run pnpm test at the top-level of the repo you should see it.

I checked and turf-isobands uses the area function to sort its resulting isoband polygons by area, so the sort order for the matrix2 test output specifically has changed, I'm sure for the better.

The solution is to update the turf-isobands test fixtures to the new correct output in your PR. Would you be willing to give it a try? Commands are as follows. I verified it worked for me.

cd packages/turf-isobands
pnpm test                             // verify matrix2 test fails
REGEN=true pnpm test        // regenerate the 4 test fixtures for turf-isobands package in test/out directory
pnpm test                             // verify all tests in package now pass
cd ../..
pnpm test                             // verify all tests in all packages now pass

Once finished, you can add the updated fixtures to your Pull Request. Ping me and I will rerun the CI testing, and hopefully we'll be good to go.

[Abdumbo99]

@twelch, Should be good to go now I guess?

[twelch]

Thank you for seeing this through @Abdumbo99 .

[twelch]

matrix2 is the only fixture that had a slightly different order on 2 very small polygons. The other fixtures appear to be the same, only the json formatting changed, adding a lot of additional lines

[Abdumbo99]

Thank you for the help @twelch . Happy to know nothing major changed!