Skip to content
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

ENH: add coverage_simplify exposing topological simplification of coverages #1969

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

martinfleis
Copy link
Contributor

xref #1570

GEOS supports this simplification for collections of polygons only. Here, I have currently opted for returning other geometries untouched but the correct behaviour is up to a discussion. When checking for correct geom types, nesting og GeometryCollections is not supported and a GeometryCollection of a GeometryCollections will not be simplified no matter the contents of the inner collection.

If we did not do this check, when points or line strings are passed, shapely returns (empty) GEOS Exception. When passing GeometryCollection with points or line strings, it kills the kernel, so I suppose that the checks I have included now are quite a feasible way forward.

I am mirroring the API of PostGIS, not that of GEOS. So the keyword is simplify_boundary rather than preserve_boundary which seems more intuitive.

@coveralls
Copy link

coveralls commented Jan 22, 2024

Pull Request Test Coverage Report for Build 8298446735

Details

  • 9 of 10 (90.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 88.122%

Changes Missing Coverage Covered Lines Changed/Added Lines %
shapely/constructive.py 9 10 90.0%
Totals Coverage Status
Change from base Build 8271573273: 0.3%
Covered Lines: 2619
Relevant Lines: 2972

💛 - Coveralls

Copy link
Collaborator

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @martinfleis !

It would be good to report test cases that crash the kernel to GEOS; those should be handled upstream.

I assume you are going to expand the test cases?

shapely/constructive.py Outdated Show resolved Hide resolved
shapely/constructive.py Outdated Show resolved Hide resolved
shapely/constructive.py Outdated Show resolved Hide resolved
src/ufuncs.c Outdated Show resolved Hide resolved
@martinfleis
Copy link
Contributor Author

I'll fish those cases out and report to GEOS, good point.

I actually wrote more tests but apparently did not push the commit. Will do once I'll get to laptop.

@theroggy
Copy link
Contributor

I suppose that the input coverage needs to be entirely topologically sound? Eg. that if two polygons touch not in a common point but a point touches a vector of another polygon between two points, this will not be treated as a common boundary?

If this is the case, I think it will be useful to explicitly mention this in the doc to avoid misunderstandings...

@jorisvandenbossche
Copy link
Member

We also need to decide how to treat the input geometry argument. AFAIK in the current state of the PR, you just pass each input geometry to the GEOS function, and so this assumes that the user already collected their set of polygons into a collection.

Alternatively, we can also do that for the user. Similarly as ST_CoverageSimplify is a window function (thus taking a set of geometries to simplify together), and similarly as for union_all we also collect the input geometries in a collection first before passing to GEOSUnaryUnion.

@martinfleis
Copy link
Contributor Author

That's a good point. I initially wanted to do that on the GeoPandas side (collect, simplify, explode) but it may make sense to do the first two steps (collect and simplify) already in shapely and return the collection. I think this covers majority of use cases and if you want to simplify per geometry, you can always loop.

I'll leave this decision to you as you have a better feeling for what the optimal behaviour of shapely is.

(Another example of this is polygonize.)

@martinfleis
Copy link
Contributor Author

I have changed the behaviour to follow the logic of union_all or ST_CoverageSimplify to make the function operate on the whole array and return a collection. The axis keyword is not yet tested but please have a look if this makes more sense API-wise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants