Add polygon area calculation #21

captainbarbosa · 2017-11-10T15:25:37Z

Closes #20 by adding the ability to calculate the area of a polygon.

Turf.js reference: https://github.com/Turfjs/turf/blob/master/packages/turf-area/index.js

frederoni · 2017-11-10T15:42:48Z

Turf/Turf.swift

+    var radius:Float = 6378137
+    var coordinates: [CLLocationCoordinate2D]
+
+    var polygonArea: Float {


Let's change all Float to Double so we don't have to cast when converting to radians.

frederoni · 2017-11-10T15:44:28Z

Turf/Turf.swift

+                p3 = coordinates[upperIndex]
+                area += (p3.longitude.toRadians() - p1.longitude.toRadians()) * sin(p2.latitude.toRadians())
+            }
+            var RADIUS = 6378137


nit: radius is already defined in the Polygon struct.

frederoni · 2017-11-10T15:49:06Z

Turf/Turf.swift

@@ -285,3 +285,65 @@ public struct Polyline {
        return closestCoordinate
    }
 }
+
+public struct Polygon {
+    var radius:Float = 6378137


Let's rename this to equatorialRadius for clarity and move it to the top where metersPerRadian is.

frederoni · 2017-11-10T15:50:01Z

We should probably migrate this project to an Xcode 9 stack on Bitrise.

frederoni · 2017-11-22T09:27:41Z

Turf/Turf.swift

@@ -285,3 +286,61 @@ public struct Polyline {
        return closestCoordinate
    }
 }
+
+public struct Polygon {
+    var coordinates: [CLLocationCoordinate2D]


~~Capturing from chat: The coordinates property on a GeoJSON Polygon should be represented as a [[CLLocationCoordate2D]]~~

Even better now encapsulated in a Ring.

1ec5 · 2017-12-08T18:52:22Z

Turf/Turf.swift

@@ -4,7 +4,10 @@ public typealias LocationRadians = Double
 public typealias RadianDistance = Double
 public typealias RadianDirection = Double

-let metersPerRadian = 6_373_000.0
+struct Constants {


I’m not sure a generic Constants struct adds much value to the file, since these constants are internal to the library.

1ec5 · 2017-12-08T18:53:14Z

Turf/Turf.swift

-let metersPerRadian = 6_373_000.0
+struct Constants {
+    static let metersPerRadian = 6_373_000.0
+    static let equatorialRadius:Double = 6378137


Specify CLLocationDistance (which is always expressed in meters) as the type of both constants.

1ec5 · 2017-12-08T18:54:46Z

Turf/Turf.swift

+    var polygonArea: Double {
+        var area:Double = 0
+
+        if (!polygonCoordinates.isEmpty && polygonCoordinates.count > 0) {


Nit: drop the parentheses.

1ec5 · 2017-12-08T18:55:59Z

Turf/Turf.swift

+    var polygonArea: Double {
+        var area:Double = 0
+
+        if (!polygonCoordinates.isEmpty && polygonCoordinates.count > 0) {


The more idiomatic way to express this in Swift is if let outerRing = polygonCoordinates.first, which allows you to avoid the [0] on the next line.

If we use a non-optional outerRing properties as in #21 (comment), then we can assume there’s an outer ring and won’t need the if statement at all.

1ec5 · 2017-12-08T19:02:30Z

Turf/Turf.swift

+            area += abs(ringArea(polygonCoordinates[0]))
+
+            for coordinate in polygonCoordinates.suffix(from: 1) {
+                area -= abs(ringArea(coordinate))


Note that each item in polygonCoordinates represents a ring’s coordinates, not an individual CLLocationCoordinate2D.

A more functional approach would be:

let area = abs(ringArea(polygonCoordinates.first ?? [])) - polygonCoordinates.suffix(from: 1) .map { abs(ringArea($0)) } // convert the inner rings to their areas .reduce(0, +) // sum the inner areas

If we use separate outerRing and innerRings properties as in #21 (comment), then this simplifies further to:

let area = abs(outerRing.area) - innerRings .map { abs($0.area) } // convert the inner rings to their areas .reduce(0, +) // sum the inner areas

1ec5 · 2017-12-08T19:11:26Z

Turf/Turf.swift

+        let coordinatesCount: Int = coordinates.count
+
+        if (coordinatesCount > 2) {
+            for index in 0...coordinatesCount - 1 {


0..<coordinatesCount would be a little clearer.

1ec5 · 2017-12-08T19:14:46Z

Turf/Turf.swift

+public struct Polygon {
+    var polygonCoordinates: [[CLLocationCoordinate2D]]
+
+    var polygonArea: Double {


Add a comment with a permalink to the specific commit of the Turf package you’re porting this code from, in case we need to track down a discrepancy in the future.

1ec5 · 2017-12-08T19:16:27Z

Turf/Turf.swift

+        var p3: CLLocationCoordinate2D
+        var lowerIndex: Int
+        var middleIndex: Int
+        var upperIndex: Int


These six variables are always written inside each iteration of the for loop and read immediately thereafter, and never read outside the loop. Move these variables to inside the loop. The original turf-area implementation declares these variables upfront to avoid hoisting, but it isn’t even necessary to do so in modern, strict-mode JavaScript.

1ec5 · 2017-12-08T19:23:27Z

Turf/Turf.swift

+        var p3: CLLocationCoordinate2D
+        var lowerIndex: Int
+        var middleIndex: Int
+        var upperIndex: Int


Use a tuple consisting of three coordinates to ensure that all three are set simultaneously:

let controlPoints: (CLLocationCoordinate2D, CLLocationCoordinate2D, CLLocationCoordinate2D) if index == coordinatesCount - 2 { controlPoints = (coordinates[coordinatesCount - 2], coordinates[coordinatesCount - 1], coordinates[0]) } // …

1ec5 · 2017-12-08T19:23:37Z

Turf/Turf.swift

+                area += (p3.longitude.toRadians() - p1.longitude.toRadians()) * sin(p2.latitude.toRadians())
+            }
+
+            area = area * Constants.equatorialRadius * Constants.equatorialRadius / 2


Nit: use the *= operator.

1ec5 · 2017-12-15T16:03:51Z

Turf/Turf.swift

-        var lowerIndex: Int
-        var middleIndex: Int
-        var upperIndex: Int
+    internal func area() -> Double {


Turn this method into a computed property to reduce the friction of using it.

1ec5 · 2017-12-15T16:04:51Z

Turf/Turf.swift

-}
+
+let metersPerRadian: CLLocationDistance = 6_373_000.0
+let equatorialRadius: CLLocationDistance = 6378137


Nit: similar to in metersPerRadian, use underscores to group the digits.

1ec5 · 2017-12-15T16:15:02Z

Turf/Turf.swift

-}
+
+let metersPerRadian: CLLocationDistance = 6_373_000.0
+let equatorialRadius: CLLocationDistance = 6378137


equatorialRadius is currently set to the equatorial radius according to WGS84 and the IUGG. This value is used in OSRM and mbgl for Web Mercator calculations.

Since different radii are used in different formulas and the errors can sometimes build up, let’s document which is which.

1ec5 · 2017-12-15T16:44:30Z

Turf/Turf.swift

-    static let equatorialRadius:Double = 6378137
-}
+
+let metersPerRadian: CLLocationDistance = 6_373_000.0


Apparently this value of 6 373 000 m came from an old version of turf-distance that I originally ported to an internal Swift application, before it got copied into another internal Swift application, then copied into the navigation SDK, then moved here. It’s close to the value of 6 372 797.560 856 that Osmium describes as “Earth’s quadratic mean radius for WGS84”.

These days, Turf.js uses a spherical approximation of 6 671 008.8 m for its radian-to-meter conversion and Haversine formula. The Haversine formula was always meant to be used with a spherical meters-per-radian value. We should probably change this value to match Turf.js. 🙀

/ref Turfjs/turf#978 Turfjs/turf#1012 Turfjs/turf#1176
/cc @frederoni @bsudekum

Since the polygon area calculation doesn’t depend on metersPerRadian, we can treat this change as tail work: #26.

If we're going to change metersPerRadian to 6 671 008.8 m, should this happen in a different PR since it would globally impact this library? Or are you ok with me making the change here?

Yeah, let’s do it in a separate PR for #26, so that any fallout is easier to track down.

In the meantime, can you add some comments explaining why these two constants differ?

1ec5 · 2017-12-15T16:51:31Z

TurfTests/TurfTests.swift


-        let polygon = Polygon(polygonCoordinates: coordinates)
+        let polygon = Polygon(outerRing: outerRing, innerRings: [])


What about the inner rings in allRings?

1ec5 · 2017-12-20T21:11:00Z

Turf/Turf.swift

-    static let equatorialRadius:Double = 6378137
-}
+
+let metersPerRadian: CLLocationDistance = 6_373_000.0


Yeah, let’s do it in a separate PR for #26, so that any fallout is easier to track down.

In the meantime, can you add some comments explaining why these two constants differ?

1ec5 · 2017-12-20T21:13:11Z

TurfTests/TurfTests.swift

@@ -295,9 +295,10 @@ class TurfTests: XCTestCase {
           $0.map { CLLocationCoordinate2D(latitude: $0[1], longitude: $0[0]) }
        }
        let outerRing = Ring(coordinates: allRings.first!)
+        let innerRing = Ring(coordinates: allRings[1])


innerRings can be expressed as allRings.prefix(from: 1) – handy for dealing with a two-hole polygon.

It looks like prefix(from:) isn't available in Swift 4.

Ooooo I think you meant suffix(from: 1) 😉

Yeah that gets me every time. 😄

1ec5 · 2018-01-12T11:48:56Z

91907e3 adds an entry for Polygon.area to the readme.

Begin port of polygon area function

d77070d

captainbarbosa self-assigned this Nov 10, 2017

captainbarbosa added the improvement Improvement for an existing feature. label Nov 10, 2017

captainbarbosa mentioned this pull request Nov 10, 2017

Polygon Area calculation? --> support #20

Closed

frederoni reviewed Nov 10, 2017

View reviewed changes

captainbarbosa added 2 commits November 21, 2017 13:36

Use doubles instead of floats

e340401

Add test fixtures + preliminary test

d89c654

frederoni reviewed Nov 22, 2017

View reviewed changes

captainbarbosa added 6 commits November 26, 2017 16:02

Fill out polygonArea

4adfa4f

More refactoring

a56f773

More edits

0c4382e

Get working calculation

dd6eac8

Refactor

d3ead9a

Place constants in struct; more refactoring

4b4ff4e

captainbarbosa requested a review from 1ec5 December 8, 2017 14:54

1ec5 reviewed Dec 8, 2017

View reviewed changes

Feedback edits from @1ec5

944e581

1ec5 reviewed Dec 15, 2017

View reviewed changes

1ec5 mentioned this pull request Dec 15, 2017

Haversine distance should use spherical Earth radius #26

Open

captainbarbosa added 2 commits December 20, 2017 09:37

Feedback edits; test still failing

047fe58

Actually update the test value 🙈

dea04ac

1ec5 approved these changes Dec 20, 2017

View reviewed changes

captainbarbosa added 3 commits December 22, 2017 09:24

Minor refactor

ead6d97

Documentation updates

51f86dc

Add documentation for Ring and Polygon

6e57647

frederoni approved these changes Jan 8, 2018

View reviewed changes

captainbarbosa merged commit 15fe874 into master Jan 8, 2018

captainbarbosa deleted the polygon-area branch January 8, 2018 17:59

captainbarbosa changed the title ~~[WIP] Add polygon area calculation~~ Add polygon area calculation Jan 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add polygon area calculation #21

Add polygon area calculation #21

captainbarbosa commented Nov 10, 2017

frederoni Nov 10, 2017

frederoni Nov 10, 2017

frederoni Nov 10, 2017

frederoni commented Nov 10, 2017

frederoni Nov 22, 2017 •

edited

Loading

1ec5 Dec 8, 2017

1ec5 Dec 8, 2017

1ec5 Dec 8, 2017

1ec5 Dec 8, 2017

1ec5 Dec 8, 2017

1ec5 Dec 8, 2017

1ec5 Dec 8, 2017 •

edited

Loading

1ec5 Dec 8, 2017

1ec5 Dec 8, 2017

1ec5 Dec 8, 2017

1ec5 Dec 8, 2017

1ec5 Dec 8, 2017

1ec5 Dec 15, 2017

1ec5 Dec 15, 2017

1ec5 Dec 15, 2017

1ec5 Dec 15, 2017

1ec5 Dec 15, 2017

captainbarbosa Dec 20, 2017

1ec5 Dec 20, 2017

1ec5 Dec 15, 2017

1ec5 Dec 20, 2017

1ec5 Dec 20, 2017

captainbarbosa Dec 22, 2017

captainbarbosa Dec 22, 2017

1ec5 Dec 22, 2017

1ec5 commented Jan 12, 2018


		let polygon = Polygon(polygonCoordinates: coordinates)
		let polygon = Polygon(outerRing: outerRing, innerRings: [])

Add polygon area calculation #21

Add polygon area calculation #21

Conversation

captainbarbosa commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni commented Nov 10, 2017

frederoni Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Jan 12, 2018

frederoni Nov 22, 2017 •

edited

Loading

1ec5 Dec 8, 2017 •

edited

Loading