Collection-based CAPI for coverage operations #867

pramsey · 2023-04-12T18:02:29Z

Exposing three coverage functions (global is-valid, is-valid with result collection, and simplify with result collection) using geometry collection as the input and output type.

dbaston · 2023-04-12T18:40:10Z

I'm thinking about the use use of "is this coverage valid? if not, give me the error locations." Currently that would require performing the validation twice, or requesting the errors and scanning it to see if any is non-empty.

Here is a signature that could accommodate the above scenario more directly, without creating a new object type. I'm not sure if I like it or not, curious what others think.

// return 1 if the coverage if the coverage is valid, 0 if it is invalid, 2 on error
// if coverage is invalid, and `errors` is not NULL, 
//`*errors` will point to a collection with the same number of elements as `cov`, providing the
// locations of errors associated with each element of `cov`
int GEOSCoverageIsValid(GEOSGeometry* cov, double gapWidth, GEOSGeometry** errors)

Another idea that I don't love is to have GEOSCoverageValid return an empty geometry if the coverage is valid (as opposed to a collection of invalid geometries).

pramsey · 2023-04-12T18:41:43Z

I like the optional signature more than I like the two separate ones.

brendan-ward

Thanks for pulling this together @pramsey to contrast with #866

Between the two, this API seems more reasonable for downstream users to implement (purely my own opinion though).

To implement against this, I'd need a bit more info in the docstrings, esp. for simplify. Are geometries below the tolerance simplified out and returned as empty polygons, do they collapse into triangles, etc.

It isn't immediately obvious what to do with the output of the validation step; if I get back MultiLineStrings for invalid edges, what can I do next to make edges valid? I'm assuming this is mostly just for flagging which polygons in the input are not valid and it's entirely up to the caller to figure out what to do next, but thought I'd ask in case the docstring could be expanded a bit to give suggested next steps...

capi/geos_c.h.in

brendan-ward · 2023-04-12T18:39:24Z

capi/geos_c.h.in

+*
+* \since 3.12
+*/
+extern GEOSGeometry GEOS_DLL * GEOSCoverageSimplify(


Suggested change

extern GEOSGeometry GEOS_DLL * GEOSCoverageSimplify(

extern GEOSGeometry GEOS_DLL * GEOSCoverageSimplifyVW(

(to allow for suffixes for other simplification methods later)

Ditto above question about what this returns on error.

Ditto above question about what this returns on error.

Should return NULL on error.

capi/geos_c.h.in

dr-jts · 2023-04-12T19:12:02Z

Another idea that I don't love is to have GEOSCoverageValid return an empty geometry if the coverage is valid (as opposed to a collection of invalid geometries).

Of course, I'm +1 for that idea :). Seems nice and simple, and keeps things a bit more functional.

It also works for empty coverage inputs (which have to be handled).

dr-jts · 2023-04-12T19:14:09Z

Thanks for pulling this together @pramsey to contrast with #866

Between the two, this API seems more reasonable for downstream users to implement (purely my own opinion though).

I agree, this API feels simpler and more familiar (even if less optimal - but beware premature optimization! And we have ideas for improving this in the future).

dr-jts · 2023-04-12T19:21:46Z

To implement against this, I'd need a bit more info in the docstrings, esp. for simplify. Are geometries below the tolerance simplified out and returned as empty polygons, do they collapse into triangles, etc.

More info can be provided. To answer: geometries never disappear, but they may be simplified down to just a triangle.
Also, some invalid geoms (such as Polygons which have too few non-repeated points) will be returned unchanged (garbage-in-garbage-out). Another thing that can be noted: if the input dataset is not a valid coverage due to overlaps, it will still be simplified, but invalid topology will still be invalid.

It isn't immediately obvious what to do with the output of the validation step; if I get back MultiLineStrings for invalid edges, what can I do next to make edges valid? I'm assuming this is mostly just for flagging which polygons in the input are not valid and it's entirely up to the caller to figure out what to do next, but thought I'd ask in case the docstring could be expanded a bit to give suggested next steps...

At the moment, you can do whatever you do now to fix the edges manually. :). The hope is to provide a CoverageClean function soon, which will try and fix all or most topology errors. But as no doubt you know, the scope for bad data is huge, so it's a challenge to try to handle all errors (and to avoid making things worse).

pramsey · 2023-04-12T21:39:19Z

How do you feel about the current state?

capi/geos_c.h.in

dbaston · 2023-04-12T21:48:53Z

How do you feel about the current state?

I like it. Thanks for the discussion and willingness to iterate.

capi/geos_ts_c.cpp

dbaston · 2023-04-12T23:32:27Z

capi/geos_ts_c.cpp

+    {
+        using geos::coverage::CoverageValidator;
+
+        return execute(extHandle, 0, [&]() {


Suggested change

return execute(extHandle, 0, [&]() {

return execute(extHandle, 2, [&]() {

dr-jts · 2023-04-12T23:43:40Z

capi/geos_c.h.in

@@ -2005,6 +2026,7 @@ extern void GEOS_DLL GEOSFree_r(
 * This function does not have a reentrant variant and is
 * available if `GEOS_USE_ONLY_R_API` is defined.
 * \return version string
+* \since 2.2


Any reason all these since annotations are in this PR?

brendan-ward · 2023-04-13T00:45:09Z

This looks good to me too, thanks for iterating on this. Excited to start using this!

Expose the CoverateSimplifier and CoverageValidator via the CAPI

Add three new functions, ST_CoverageIsValid(geom geometry winset, tolerance float8) ST_CoverageSimplify(geom geometry winset, tolerance float8, simplifyBoundary integer) ST_CoverageUnion(geom geometry set) For the purpose of these functions a "coverage" is a set of polygons with perfectly shared edges (no overlaps or gaps and exact vertex matching). For associated GEOS discussion, see libgeos/geos#867

ST_CoverageIsValid(geom geometry winset, tolerance float8) ST_CoverageSimplify(geom geometry winset, tolerance float8, simplifyBoundary integer) ST_CoverageUnion(geom geometry set) For the purpose of these functions a "coverage" is a set of polygons with perfectly shared edges (no overlaps or gaps and exact vertex matching). For associated GEOS discussion, see libgeos/geos#867

pramsey mentioned this pull request Apr 12, 2023

CAPI for coverage operations #866

Closed

brendan-ward reviewed Apr 12, 2023

View reviewed changes

dr-jts reviewed Apr 12, 2023

View reviewed changes

capi/geos_c.h.in Outdated Show resolved Hide resolved

pramsey added 3 commits April 12, 2023 13:06

collection-based API example

fa20e02

doxy

3db6736

Incorporate feedback from commenters

fb4a039

pramsey force-pushed the main-coverage-capi-gc branch from 5db0a2b to fb4a039 Compare April 12, 2023 20:07

pramsey added 2 commits April 12, 2023 13:08

note on error return

ce6b898

Add VW suffix to Simplify

2588646

pramsey requested a review from dbaston April 12, 2023 21:39

dbaston approved these changes Apr 12, 2023

View reviewed changes

capi/geos_c.h.in Show resolved Hide resolved

capi/geos_c.h.in Outdated Show resolved Hide resolved

capi/geos_c.h.in Outdated Show resolved Hide resolved

capi/geos_c.h.in Show resolved Hide resolved

pramsey added 2 commits April 12, 2023 14:55

incorporate @dbaston review

ba407e4

Finish up marking since on CAPI, closes libgeos#854

d513c09

dbaston reviewed Apr 12, 2023

View reviewed changes

capi/geos_ts_c.cpp Outdated Show resolved Hide resolved

throw exception rather than returning error code

9c23ff0

dbaston reviewed Apr 12, 2023

View reviewed changes

dr-jts reviewed Apr 12, 2023

View reviewed changes

add a capi test

9c94290

pramsey added a commit that referenced this pull request Apr 13, 2023

CAPI: GEOSCoverageIsValid, GEOSCoverageSimplifyVW GH-867

3cab6cb

Expose the CoverateSimplifier and CoverageValidator via the CAPI

pramsey closed this Apr 13, 2023

pramsey deleted the main-coverage-capi-gc branch April 24, 2023 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection-based CAPI for coverage operations #867

Collection-based CAPI for coverage operations #867

pramsey commented Apr 12, 2023

dbaston commented Apr 12, 2023

pramsey commented Apr 12, 2023

brendan-ward left a comment

brendan-ward Apr 12, 2023

dr-jts Apr 12, 2023

dr-jts commented Apr 12, 2023

dr-jts commented Apr 12, 2023

dr-jts commented Apr 12, 2023

pramsey commented Apr 12, 2023

dbaston commented Apr 12, 2023

dbaston Apr 12, 2023

dr-jts Apr 12, 2023

pramsey Apr 13, 2023

brendan-ward commented Apr 13, 2023

	extern GEOSGeometry GEOS_DLL * GEOSCoverageSimplify(
	extern GEOSGeometry GEOS_DLL * GEOSCoverageSimplifyVW(

	return execute(extHandle, 0, [&]() {
	return execute(extHandle, 2, [&]() {

Collection-based CAPI for coverage operations #867

Collection-based CAPI for coverage operations #867

Conversation

pramsey commented Apr 12, 2023

dbaston commented Apr 12, 2023

pramsey commented Apr 12, 2023

brendan-ward left a comment

Choose a reason for hiding this comment

brendan-ward Apr 12, 2023

Choose a reason for hiding this comment

dr-jts Apr 12, 2023

Choose a reason for hiding this comment

dr-jts commented Apr 12, 2023

dr-jts commented Apr 12, 2023

dr-jts commented Apr 12, 2023

pramsey commented Apr 12, 2023

dbaston commented Apr 12, 2023

dbaston Apr 12, 2023

Choose a reason for hiding this comment

dr-jts Apr 12, 2023

Choose a reason for hiding this comment

pramsey Apr 13, 2023

Choose a reason for hiding this comment

brendan-ward commented Apr 13, 2023