feat(geos): Add `Geometry#voronoi_diagram` #334

BuonOmo · 2022-10-11T12:25:33Z

Fixes #93

BuonOmo · 2022-10-13T13:31:40Z

ext/geos_c_impl/geometry.c

+  if (self_geom) {
+    if (RB_TEST(env)) {
+      Check_TypedStruct(env, &rgeo_geometry_type);
+      env_geom = rgeo_convert_to_geos_geometry(self_data->factory, env, Qnil);


This method could return NULL. In the spirit of v3, shouldn't we raise in stead?

Is that how we handle other GEOS methods that return NULL? We raise a GeosError now?

It is not, but that's my point! I think this debate should be part of another PR anyway. But here it can affect the behaviour:

I run geom.voronoi_diagram(envelope: poly), and I expect the envelope to be poly, but since the conversion silently failed, the envelope param is actually ignored.

I can solve the local issue by raising in case of null value, and solve the global problem in a later PR. Or just do nothing here and accept the potential silent failure here, which is anyway not that likely to happen (Feature.cast would need to return nil)

Actually the fix would just be to remove those two lines (and check that this method raising is not an issue in the codebase, but last time I checked it was OK, and memcheck is OK as well):

rgeo/ext/geos_c_impl/factory.c

Lines 834 to 835 in ea9b1d7

if (NIL_P(object))

return NULL;

I can solve the local issue by raising in case of null value, and solve the global problem in a later PR

I think this is the best approach. I agree that this behavior should be the standard since it's more obvious that there was an issue. Of course it's still a similar amount of work for the user since they have to rescue now instead of checking for nil but I think being more explicit and actually raising instead of silently failing is better.

I have added a local security for now, I'm thinking of quickly remove it in favor of the global solution. Otherwise this error could be present in various places in the code base

BuonOmo · 2022-10-13T13:32:56Z

lib/rgeo/geos/capi_feature_classes.rb

+      #   if +only_edges+ is true, this will return a LineString or MultiLineString.
+      #   Otherwise, it will be a GeometryCollection of polygons.


The fact that it has 3 different possible return values may be bothering, WDYT?

I think that's fine. There's other methods like union that will return different types depending on the resultant geometry graph so this isn't unusual. Does accounting for the different types make it more difficult to handle in the method?

Not at all. It may just make it more difficult in the usage. Especially the linestring vs multilinestring seems tricky. But to be fair, this may be a geos issue since it also affects geos and shapely!

libgeos/geos#702 Geos PR

**change** the getDiagramEdges now only returns a MultiLineString, whether there is one or many lines. Which reduces to two the number of possible kind of geometries given by `GEOSVoronoiDiagram` **reason** I've been working on adding `voronoi_diagram` to RGeo. And looking at the [Shapely PR for voronoi diagram][1] and [mine in RGeo][2], it seems like the `GEOSVoronoiDiagram` CAPI method causes confusion by returning three possible kind of geometries: - `GeometryCollection` (when `onlyEdges` is false) - `MultiLineString` (`onlyEdges` is true and there are at least two lines) - `LineString` (`onlyEdges` is true and there is only one line) The parameter `onlyEdges` changes the outcome, which is ok to reason about. However, the fact that we could have either a LineString or a MultiLineString is way harder to work around in my opinion. [1]: shapely/shapely#881 (comment) [2]: rgeo/rgeo#334 (comment)

**change** the getDiagramEdges now only returns a GeometryCollection, whether there is one or many lines. So `GEOSVoronoiDiagram` now can only return a GeometryCollection itself. **reason** I've been working on adding `voronoi_diagram` to RGeo. And looking at the [Shapely PR for voronoi diagram][1] and [mine in RGeo][2], it seems like the `GEOSVoronoiDiagram` CAPI method causes confusion by returning three possible kind of geometries: - `GeometryCollection` (when `onlyEdges` is false) - `MultiLineString` (`onlyEdges` is true and there are at least two lines) - `LineString` (`onlyEdges` is true and there is only one line) The fact that we could have either a LineString or a MultiLineString is quite confusing. And wrapping the whole result in a GeometryCollection is actually [not a significant overhead][3]. Hence the choice of always returning a GeometryCollection. [1]: shapely/shapely#881 (comment) [2]: rgeo/rgeo#334 (comment) [3]: libgeos#702 (comment)

BuonOmo · 2022-10-19T13:28:16Z

lib/rgeo/geos/capi_feature_classes.rb

@@ -101,7 +101,7 @@ def as_text
      # @see https://en.wikipedia.org/wiki/Voronoi_diagram
      # @see https://libgeos.org/doxygen/geos__c_8h.html#ace0b2fabc92d8457a295c385ea128aa5
      def voronoi_diagram(envelope: nil, tolerance: 0.0, only_edges: false)
-        Primary.voronoi_diagram(self, envelope, tolerance || 0.0, only_edges)
+        Primary.voronoi_diagram(self, envelope, Float(tolerance), only_edges)


The change here allows one to pass any kind of numeric

BuonOmo · 2022-10-19T13:39:16Z

Damn, I'd love to say that we're ready to merge, but I think there's a leak:

require "get_process_mem"
trap(:INFO){p GetProcessMem.new.mb}
po=RGeo::Geos.factory.parse_wkt("POLYGON ((0 0, 0.5 0, 0.5 0.5, 0 0.5, 0 0))")
loop{po.voronoi_diagram(tolerance: 0.6)rescue nil}

hitting ctrl+t regularly (or kill -INFO <pid>) shows an ever increasing memory.

keithdoggett

Looks good. I just left a few questions. Primarily related to error handling and what types of errors we should be raising from Geos. I'd prefer to have errors from Geos raise GeosError instead of InvalidGeometry because with the validity changes it might be confusing to get InvalidGeometry errors on geometry creation when we've stated the intention is that will no longer happen.

keithdoggett · 2022-10-27T12:16:03Z

ext/geos_c_impl/geometry.c

+    if (diagram == NULL) {
+      rb_raise(rb_eGeosError, "GEOS cannot create a voronoi diagram");
+    }


should we remove this and let the handler that we're going to implement in the wrappers take care of raising?

keithdoggett · 2022-10-27T12:18:10Z

lib/rgeo/geos/capi_feature_classes.rb

+      rescue RGeo::Error::InvalidGeometry => e
+        message = "Could not create a voronoi_diagram with the specified inputs"
+        message += ". Try removing the `tolerance` parameter from ##{__method__}" if tolerance
+        raise e, message
+      end


when would this get raised?

keithdoggett · 2022-10-27T12:18:49Z

lib/rgeo/geos/ffi_feature_methods.rb

+      rescue ::Geos::GEOSException
+        message = "Could not create a voronoi_diagram with the specified inputs"
+        message += ". Try removing the `tolerance` parameter from ##{__method__}" if tolerance
+        raise RGeo::Error::InvalidGeometry, message


should we raise an InvalidGeometry error or a GeosError here?

**change** the getDiagramEdges now only returns a GeometryCollection, whether there is one or many lines. So `GEOSVoronoiDiagram` now can only return a GeometryCollection itself. **reason** I've been working on adding `voronoi_diagram` to RGeo. And looking at the [Shapely PR for voronoi diagram][1] and [mine in RGeo][2], it seems like the `GEOSVoronoiDiagram` CAPI method causes confusion by returning three possible kind of geometries: - `GeometryCollection` (when `onlyEdges` is false) - `MultiLineString` (`onlyEdges` is true and there are at least two lines) - `LineString` (`onlyEdges` is true and there is only one line) The fact that we could have either a LineString or a MultiLineString is quite confusing. And wrapping the whole result in a GeometryCollection is actually [not a significant overhead][3]. Hence the choice of always returning a GeometryCollection. [1]: shapely/shapely#881 (comment) [2]: rgeo/rgeo#334 (comment) [3]: libgeos#702 (comment)

Improve consistency of GEOSVoronoiDiagram **change** the getDiagramEdges now only returns a MultiLineString, whether there is one or many lines. Which reduces to two the number of possible kind of geometries given by `GEOSVoronoiDiagram` **reason** I've been working on adding `voronoi_diagram` to RGeo. And looking at the [Shapely PR for voronoi diagram][1] and [mine in RGeo][2], it seems like the `GEOSVoronoiDiagram` CAPI method causes confusion by returning three possible kind of geometries: - `GeometryCollection` (when `onlyEdges` is false) - `MultiLineString` (`onlyEdges` is true and there are at least two lines) - `LineString` (`onlyEdges` is true and there is only one line) The parameter `onlyEdges` changes the outcome, which is ok to reason about. However, the fact that we could have either a LineString or a MultiLineString is way harder to work around in my opinion. [1]: shapely/shapely#881 (comment) [2]: rgeo/rgeo#334 (comment)

lib/rgeo/geos/capi_feature_classes.rb

BuonOmo force-pushed the voronoi-diagram branch 2 times, most recently from 61c5cc3 to 1fc06b1 Compare October 11, 2022 14:57

BuonOmo commented Oct 13, 2022

View reviewed changes

BuonOmo added 2 commits October 13, 2022 15:35

feat(geos): Add Geometry#voronoi_diagram

ea9b1d7

add ffi + tests

0ae5fb4

BuonOmo force-pushed the voronoi-diagram branch from a379146 to 0ae5fb4 Compare October 13, 2022 13:35

BuonOmo added 3 commits October 13, 2022 15:36

ruby 2.6

8ec0388

fix compilation warnings

01a4f16

rubocop

ca01ff7

BuonOmo force-pushed the voronoi-diagram branch 2 times, most recently from 4b2a80c to be28284 Compare October 13, 2022 13:53

fix leak

3fa838d

BuonOmo force-pushed the voronoi-diagram branch from be28284 to 3fa838d Compare October 13, 2022 13:53

fix leak 2

fc01fce

BuonOmo force-pushed the voronoi-diagram branch from 32d5023 to fc01fce Compare October 14, 2022 12:00

no need to check type, already done by the conversion method

f943d92

BuonOmo mentioned this pull request Oct 14, 2022

Change GEOSVoronoiDiagram to return edges as MultiLineString libgeos/geos#702

Merged

BuonOmo commented Oct 19, 2022

View reviewed changes

BuonOmo marked this pull request as ready for review October 19, 2022 13:28

add check for envelope result

f82709a

BuonOmo force-pushed the voronoi-diagram branch from abc6997 to f82709a Compare October 19, 2022 13:30

keithdoggett approved these changes Oct 27, 2022

View reviewed changes

BuonOmo commented Nov 12, 2022

View reviewed changes

lib/rgeo/geos/capi_feature_classes.rb Outdated Show resolved Hide resolved

Update lib/rgeo/geos/capi_feature_classes.rb

0c5eb1f

keithdoggett mentioned this pull request Oct 20, 2023

feat: Segmentize for GeosCapi geometries. #364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(geos): Add `Geometry#voronoi_diagram` #334

feat(geos): Add `Geometry#voronoi_diagram` #334

BuonOmo commented Oct 11, 2022 •

edited

BuonOmo Oct 13, 2022

keithdoggett Oct 13, 2022

BuonOmo Oct 14, 2022 •

edited

keithdoggett Oct 14, 2022

BuonOmo Oct 19, 2022

BuonOmo Oct 13, 2022

keithdoggett Oct 13, 2022

BuonOmo Oct 14, 2022

BuonOmo Oct 14, 2022

BuonOmo Oct 19, 2022

BuonOmo commented Oct 19, 2022

keithdoggett left a comment

keithdoggett Oct 27, 2022

keithdoggett Oct 27, 2022

keithdoggett Oct 27, 2022

		# if +only_edges+ is true, this will return a LineString or MultiLineString.
		# Otherwise, it will be a GeometryCollection of polygons.

feat(geos): Add Geometry#voronoi_diagram #334

Are you sure you want to change the base?

feat(geos): Add Geometry#voronoi_diagram #334

Conversation

BuonOmo commented Oct 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BuonOmo Oct 14, 2022 • edited

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

BuonOmo commented Oct 19, 2022

keithdoggett left a 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

feat(geos): Add `Geometry#voronoi_diagram` #334

feat(geos): Add `Geometry#voronoi_diagram` #334

BuonOmo commented Oct 11, 2022 •

edited

BuonOmo Oct 14, 2022 •

edited