polygonToCells API accepts a flags argument (#570)

* polygonToCells API accepts a flags argument * Fix build * Fix build * Fix test
uber · Feb 4, 2022 · 28b7107 · 28b7107
1 parent adde6da
commit 28b7107
Show file tree

Hide file tree

Showing 9 changed files with 215 additions and 73 deletions.
diff --git a/dev-docs/RFCs/v4.0.0/polyfill-modes-rfc.md b/dev-docs/RFCs/v4.0.0/polyfill-modes-rfc.md
@@ -1,7 +1,7 @@
 # RFC: Polyfill modes
 
-* **Authors**: -
-* **Date**: -
+* **Authors**: Isaac Brodsky (@isaacbrodsky)
+* **Date**: September 17, 2021
 * **Status**: Draft
 
 ## Abstract
@@ -12,10 +12,118 @@ Our current polyfill algorithm allocates cells to polygons based on whether the
 
 *Why is this important?*
 
+* Inclusion test
+
+A use case we want to support is testing whether a point could possibly be in a polygon by set containment. In Java this would look like:
+
+```java
+final int R = ...;
+// Indexed as res R
+Set<String> cellsCoveringPolygon = ...;
+
+public boolean polygonContainsPoint(double latitude, double longitude) {
+    String h3Index = h3.latLngToCell(latitude, longitude, R);
+    if (cellsCoveringPolygon.contains(h3Index)) {
+        // The polygon possibly contains the point, so use a more expensive point
+        // in polygon algorithm to determine.
+        return polygonContainsPointExpensive(latitude, longitude);
+    }
+    // The polygon definitely does not contain the point, the expensive check
+    // is not needed.
+    return false;
+}
+```
+
+* Analytics
+
+There is no convenient function to index a polygon into the H3 grid for analytics purposes. Polygons that are small enough or shaped in specific ways will not be converted into cells, requiring workarounds in order to use them.
+
+* Spherical geometry consistency
+
+Most of the H3 library uses spherical geoemtry. For example, cell boundaries are spherical hexagons
+and pentagons. The `polyfill` function is different that it assumes Cartesian geometry. For
+consistency with the rest of the library, the `polyfill` functions should be able to use the same
+cell boundaries.
+
+Maintaining a Cartesian option is useful for cases where polygons have been drawn on a projected map
+and the boundaries should be the same.
+
+* Very large polygons
+
+Polyfills of very large polygons require allocating large blocks of memory, and spending large
+amounts of time in a library function without progress information being available to the caller.
+(To be determined if this is in scope for this RFC or for another.)
+
 ## Approaches
 
 *What are the various options to address this issue?*
 
+On an API level, we will need to expose the containment mode and spherical/Cartesian choice as
+options to the caller.
+
+Internally, we would like to reuse the same implementation as much as possible, but change the
+exact inclusion check used for each mode.
+
+### Comparisons
+
+#### S2
+
+Contains separate functions for [intersection and containment](http://s2geometry.io/devguide/basic_types#s2polygon)
+
+#### Turf library
+
+Contains separate functions for [intersection](http://turfjs.org/docs/#booleanIntersects), [containment](http://turfjs.org/docs/#booleanContains), etc.
+
+#### PostGIS
+
+Contains separate functions for [intersection](https://postgis.net/docs/ST_Intersects.html), [containment](https://postgis.net/docs/ST_Contains.html), etc.
+
+See also [Simple Feature Access - SQL](https://www.ogc.org/standards/sfs).
+
+#### JTS
+
+Separate predicatess. (Reference: [JTS Developer Guide](https://github.com/locationtech/jts/blob/master/doc/JTS%20Developer%20Guide.pdf))
+
+#### GeoPandas
+
+Contains separate functions for [intersection](https://geopandas.org/en/stable/docs/reference/api/geopandas.GeoSeries.intersection.html), [containment](https://geopandas.org/en/stable/docs/reference/api/geopandas.GeoSeries.contains.html), etc.
+
 ## Proposal
 
-*What is the recommended approach?*
+*What is the recommended approach?*
+
+The signature for `polygonToCells` and `maxPolygonToCellsSize` would be changed as follows:
+
+```
+/** @brief maximum number of hexagons that could be in the geoloop */
+DECLSPEC H3Error H3_EXPORT(maxPolygonToCellsSize)(const GeoPolygon *geoPolygon,
+                                                  int res, uint32_t flags, int64_t *out);
+
+/** @brief hexagons within the given geopolygon */
+DECLSPEC H3Error H3_EXPORT(polygonToCells)(const GeoPolygon *geoPolygon,
+                                           int res, uint32_t flags, H3Index *out);
+```
+
+`flags` would have the following possible bit layout:
+
+|       Bits | Meaning
+| ---------- | -------
+| 1-2 (LSB)  | If 0, containment mode centroid.<br>If 1, containment mode cover.<br>If 2, containment mode intersects.<br>3 is a reserved value.
+| 3          | If 0, spherical containment.<br>If 1, cartesian containment (same as H3 version 3).
+| All others | Reserved and must be set to 0.
+
+The same value used for `maxPolygonToCellsSize` must be used for the subsequent call to `polygonToCells`, just as the `GeoPolygon` and `res` must be the same.
+
+In bindings, this could be expressed using enums, for example:
+
+```python
+polygon_to_cells(polygon, res=res, cartesian=True, containment=h3.Containment.CENTROID)
+```
+
+```js
+polygonToCells(polygon, {res, cartesian: true, containment: h3.Containment.CENTROID})
+```
+
+```java
+polygon(polygon).cartesian(true).containment(h3.Containment.CENTROID).toCells(res)
+```
diff --git a/src/apps/benchmarks/benchmarkPolygonToCells.c b/src/apps/benchmarks/benchmarkPolygonToCells.c
@@ -122,23 +122,23 @@ int64_t numHexagons;
 H3Index *hexagons;
 
 BENCHMARK(polygonToCellsSF, 500, {
-    H3_EXPORT(maxPolygonToCellsSize)(&sfGeoPolygon, 9, &numHexagons);
+    H3_EXPORT(maxPolygonToCellsSize)(&sfGeoPolygon, 9, 0, &numHexagons);
     hexagons = calloc(numHexagons, sizeof(H3Index));
-    H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, hexagons);
+    H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, 0, hexagons);
     free(hexagons);
 });
 
 BENCHMARK(polygonToCellsAlameda, 500, {
-    H3_EXPORT(maxPolygonToCellsSize)(&alamedaGeoPolygon, 9, &numHexagons);
+    H3_EXPORT(maxPolygonToCellsSize)(&alamedaGeoPolygon, 9, 0, &numHexagons);
     hexagons = calloc(numHexagons, sizeof(H3Index));
-    H3_EXPORT(polygonToCells)(&alamedaGeoPolygon, 9, hexagons);
+    H3_EXPORT(polygonToCells)(&alamedaGeoPolygon, 9, 0, hexagons);
     free(hexagons);
 });
 
 BENCHMARK(polygonToCellsSouthernExpansion, 10, {
-    H3_EXPORT(maxPolygonToCellsSize)(&southernGeoPolygon, 9, &numHexagons);
+    H3_EXPORT(maxPolygonToCellsSize)(&southernGeoPolygon, 9, 0, &numHexagons);
     hexagons = calloc(numHexagons, sizeof(H3Index));
-    H3_EXPORT(polygonToCells)(&southernGeoPolygon, 9, hexagons);
+    H3_EXPORT(polygonToCells)(&southernGeoPolygon, 9, 0, hexagons);
     free(hexagons);
 });
 

diff --git a/src/apps/fuzzers/fuzzerPolygonToCells.c b/src/apps/fuzzers/fuzzerPolygonToCells.c
@@ -50,12 +50,12 @@ int populateGeoLoop(GeoLoop *g, const uint8_t *data, size_t *offset,
     return 0;
 }
 
-void run(GeoPolygon *geoPolygon, int res) {
+void run(GeoPolygon *geoPolygon, uint32_t flags, int res) {
     int64_t sz;
-    H3Error err = H3_EXPORT(maxPolygonToCellsSize)(geoPolygon, res, &sz);
+    H3Error err = H3_EXPORT(maxPolygonToCellsSize)(geoPolygon, res, flags, &sz);
     if (!err && sz < MAX_SZ) {
         H3Index *out = calloc(sz, sizeof(H3Index));
-        H3_EXPORT(polygonToCells)(geoPolygon, res, out);
+        H3_EXPORT(polygonToCells)(geoPolygon, res, flags, out);
         free(out);
     }
 }
@@ -88,9 +88,10 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
         }
     }
 
-    run(&geoPolygon, res);
+    // TODO: Fuzz the `flags` input as well when it has meaningful input
+    run(&geoPolygon, 0, res);
     geoPolygon.numHoles = 0;
-    run(&geoPolygon, res);
+    run(&geoPolygon, 0, res);
     free(geoPolygon.holes);
 
     return 0;

diff --git a/src/apps/fuzzers/fuzzerPolygonToCellsNoHoles.c b/src/apps/fuzzers/fuzzerPolygonToCellsNoHoles.c
@@ -24,12 +24,12 @@
 const int MAX_RES = 15;
 const int MAX_SZ = 4000000;
 
-void run(GeoPolygon *geoPolygon, int res) {
+void run(GeoPolygon *geoPolygon, uint32_t flags, int res) {
     int64_t sz;
-    H3Error err = H3_EXPORT(maxPolygonToCellsSize)(geoPolygon, res, &sz);
+    H3Error err = H3_EXPORT(maxPolygonToCellsSize)(geoPolygon, res, flags, &sz);
     if (!err && sz < MAX_SZ) {
         H3Index *out = calloc(sz, sizeof(H3Index));
-        H3_EXPORT(polygonToCells)(geoPolygon, res, out);
+        H3_EXPORT(polygonToCells)(geoPolygon, res, flags, out);
         free(out);
     }
 }
@@ -50,7 +50,8 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
     // Offset by 1 since *data was used for `res`, above.
     geoPolygon.geoloop.verts = (LatLng *)(data + 1);
 
-    run(&geoPolygon, res);
+    // TODO: Fuzz the `flags` input as well when it has meaningful input
+    run(&geoPolygon, 0, res);
 
     return 0;
 }

diff --git a/src/apps/testapps/testH3Memory.c b/src/apps/testapps/testH3Memory.c
@@ -189,31 +189,31 @@ SUITE(h3Memory) {
         sfGeoPolygon.numHoles = 0;
 
         int64_t numHexagons;
-        t_assertSuccess(
-            H3_EXPORT(maxPolygonToCellsSize)(&sfGeoPolygon, 9, &numHexagons));
+        t_assertSuccess(H3_EXPORT(maxPolygonToCellsSize)(&sfGeoPolygon, 9, 0,
+                                                         &numHexagons));
         H3Index *hexagons = calloc(numHexagons, sizeof(H3Index));
 
         resetMemoryCounters(0);
         failAlloc = true;
-        H3Error err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, hexagons);
+        H3Error err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, 0, hexagons);
         t_assert(err == E_MEMORY, "polygonToCells failed (1)");
         t_assert(actualAllocCalls == 1, "alloc called once");
         t_assert(actualFreeCalls == 0, "free not called");
 
         resetMemoryCounters(1);
-        err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, hexagons);
+        err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, 0, hexagons);
         t_assert(err == E_MEMORY, "polygonToCells failed (2)");
         t_assert(actualAllocCalls == 2, "alloc called twice");
         t_assert(actualFreeCalls == 1, "free called once");
 
         resetMemoryCounters(2);
-        err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, hexagons);
+        err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, 0, hexagons);
         t_assert(err == E_MEMORY, "polygonToCells failed (3)");
         t_assert(actualAllocCalls == 3, "alloc called three times");
         t_assert(actualFreeCalls == 2, "free called twice");
 
         resetMemoryCounters(3);
-        err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, hexagons);
+        err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, 0, hexagons);
         t_assert(err == E_SUCCESS, "polygonToCells succeeded (4)");
         t_assert(actualAllocCalls == 3, "alloc called three times");
         t_assert(actualFreeCalls == 3, "free called three times");