Add GEOSVoronoiDiagram to shapely #881

tomplex · 2020-03-31T03:14:15Z

I thought it made sense to keep Delauney / Voronoi close together in the code since they're similar in use and right next to each other in the CAPI, but if you disagree I'm happy to move it to wherever.

I couldn't nail down for sure exactly when VoronoiDiagram was added to the CAPI. This entry was the best I could find indicating release version, so I went with 3.5.0.

coveralls · 2020-03-31T03:42:36Z

Coverage increased (+0.1%) to 81.477% when pulling 2efb189 on tomplex:voronoi-diagram into 808fd46 on Toblerity:master.

tomplex · 2020-03-31T05:24:22Z

tests/test_voronoi_diagram.py

+
+
+@requires_geos_35
+def test_from_polygon_without_enough_tolerance():


This test and the ones below it drive some un-intuitive behavior with regards to the tolerance argument. The first case is when we specify a tolerance to the function which is, I guess, "not enough"? I'm not necessarily sure how the tolerance value is associated with snapping (I need to look at the source C code more) but the C API returns a null geometry, which throws an error in the geom_factory function.

The case below is perhaps more interesting: if we specify a tolerance and we don't have any floating point coordinates, then we also get a null geometry back.

Either way, I added some code to catch the generic error about creating geometry from null values and raise an error which is generally more descriptive, but I wonder if it's worth calling this behavior out specifically in an error? Or is it overkill? Either way I also added some info to the docstrings & docs.

Jeremiah-England · 2020-03-31T20:45:56Z

shapely/ops.py

+
+def voronoi_diagram(geom, envelope=None, tolerance=0.0, edges=False):
+    """
+    Constructs a Voronoi Diagram [1] from the given geometry.


Most of this is over my head but it looks really nice. I think you need a underscore after the references in numpydocs if we want it to build with hyperlinks eventually. Like this: [1]_. And you'll also want to add a .. before each reference down in the References section.

See point number 14 here: https://numpydoc.readthedocs.io/en/latest/format.html

I'll defer to @sgillies on this, I'm happy to add the extra formatting if the docstrings will be built into documentation, but if they're mostly used for inline help (like help(voronoi_diagram)) then the extra formatting might not be helpful. Either way thanks for the suggestion! =)

shapely/ops.py

jorisvandenbossche · 2020-04-01T07:24:21Z

The current return value is a list of geometries. Could it also be considered to return the actual GeometryCollection (as it is returned by GEOS)? The user could then still get the list by doing result.geoms themselves.

I assume that for users that pass a single geometry, the current API returning a list is easier (and also more consisitent with the existing triangulate, which is a good reason of course ..). But returning a geometry collection would make it easier to have it compatible with the PyGEOS version.

tomplex · 2020-04-01T17:16:30Z

@jorisvandenbossche, though I personally agree that a GeometryCollection feels like a better option since that's what being returned natively by GEOS, I think one of the best parts of Shapely is the consistency and therefore intuitiveness of the API, so I went with a list of geometries. I'd be happy to discuss more if you feel strongly, especially because it seems like there will be a closer union of Shapely and PyGeos in the future, so it makes sense to take consistency between the two libraries into account.

tomplex · 2020-04-06T03:51:39Z

@sgillies should I rebase this onto master?

sgillies · 2020-04-06T15:39:00Z

@tomplex yes, please.

Returning a GeometryCollection would be more efficient and I'm in favor 👍

… the "right" value

…re tolerance on / off causes a failure

sgillies

@tomplex thank you!

**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)

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)

tomplex changed the title ~~WIP: Add GEOSVoronoiDiagram to shapely~~ Add GEOSVoronoiDiagram to shapely Mar 31, 2020

tomplex commented Mar 31, 2020

View reviewed changes

Jeremiah-England reviewed Mar 31, 2020

View reviewed changes

shapely/ops.py Outdated Show resolved Hide resolved

Jeremiah-England reviewed Mar 31, 2020

View reviewed changes

shapely/ops.py Outdated Show resolved Hide resolved

Jeremiah-England reviewed Mar 31, 2020

View reviewed changes

shapely/ops.py Outdated Show resolved Hide resolved

sgillies modified the milestones: Shapely 3001, 1.8 Apr 2, 2020

tomplex added 16 commits April 6, 2020 22:26

Exposing GEOSVoronoiDiagram.

186a923

Return list of geometries instead of GeometryCollection

2c12095

Add docstrings

2c3d4dd

variable rename

5dc3aa8

formatting and NULL -> None

cf234d3

add to __all__

4777ca7

Add documentation & plot, update docstrings

9107e1c

update doc title

0f68a90

documentation wording

4587e40

add unit tests

cccd4c0

typo

d70a856

add tests for tolerance

4604673

catch specific error and adjust tolerance to be more demonstrative of…

92ce18b

… the "right" value

Add note to docstrings and add more tests to drive specific cases whe…

c7408e7

…re tolerance on / off causes a failure

add more info on tolerance to the docs.

29d8d43

formatting

a3b86fe

tomplex added 2 commits April 6, 2020 22:30

format docstrings to better comply with numpy style.

a6f2bf2

de-dent and env -> envelope

47ecb18

tomplex force-pushed the voronoi-diagram branch from 80edd53 to 47ecb18 Compare April 7, 2020 02:30

tomplex changed the base branch from maint-1.7 to master April 7, 2020 02:32

tomplex added 2 commits April 6, 2020 22:41

return a GeometryCollection instead of a list of geoms

4d71be0

update docstrings to show GeometryCollection return value

2efb189

sgillies approved these changes Apr 7, 2020

View reviewed changes

sgillies merged commit da5b611 into shapely:master Apr 7, 2020

jorisvandenbossche mentioned this pull request Aug 5, 2020

Can't import voronoi_diagram #954

Closed

jorisvandenbossche mentioned this pull request Jan 14, 2021

Add Voronoi Diagram #833

Closed

BuonOmo mentioned this pull request Oct 14, 2022

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GEOSVoronoiDiagram to shapely #881

Add GEOSVoronoiDiagram to shapely #881

tomplex commented Mar 31, 2020 •

edited

Loading

coveralls commented Mar 31, 2020 •

edited

Loading

tomplex Mar 31, 2020 •

edited

Loading

Jeremiah-England Mar 31, 2020

tomplex Mar 31, 2020

jorisvandenbossche commented Apr 1, 2020

tomplex commented Apr 1, 2020 •

edited

Loading

tomplex commented Apr 6, 2020

sgillies commented Apr 6, 2020

sgillies left a comment



		@requires_geos_35
		def test_from_polygon_without_enough_tolerance():

Add GEOSVoronoiDiagram to shapely #881

Add GEOSVoronoiDiagram to shapely #881

Conversation

tomplex commented Mar 31, 2020 • edited Loading

coveralls commented Mar 31, 2020 • edited Loading

tomplex Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

Jeremiah-England Mar 31, 2020

Choose a reason for hiding this comment

tomplex Mar 31, 2020

Choose a reason for hiding this comment

jorisvandenbossche commented Apr 1, 2020

tomplex commented Apr 1, 2020 • edited Loading

tomplex commented Apr 6, 2020

sgillies commented Apr 6, 2020

sgillies left a comment

Choose a reason for hiding this comment

tomplex commented Mar 31, 2020 •

edited

Loading

coveralls commented Mar 31, 2020 •

edited

Loading

tomplex Mar 31, 2020 •

edited

Loading

tomplex commented Apr 1, 2020 •

edited

Loading