-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(geos): Add Geometry#voronoi_diagram
#334
base: main
Are you sure you want to change the base?
Conversation
61c5cc3
to
1fc06b1
Compare
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method could return NULL. In the spirit of v3, shouldn't we raise in stead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that how we handle other GEOS methods that return NULL
? We raise a GeosError
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
# if +only_edges+ is true, this will return a LineString or MultiLineString. | ||
# Otherwise, it will be a GeometryCollection of polygons. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that it has 3 different possible return values may be bothering, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libgeos/geos#702 Geos PR
a379146
to
0ae5fb4
Compare
4b2a80c
to
be28284
Compare
be28284
to
3fa838d
Compare
32d5023
to
fc01fce
Compare
**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)
**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)
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change here allows one to pass any kind of numeric
abc6997
to
f82709a
Compare
Damn, I'd love to say that we're ready to merge, but I think there's a leak:
hitting ctrl+t regularly (or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (diagram == NULL) { | ||
rb_raise(rb_eGeosError, "GEOS cannot create a voronoi diagram"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove this and let the handler that we're going to implement in the wrappers take care of raising?
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when would this get raised?
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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)
Fixes #93