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

API: how to expose coverage_union? #3117

Open
martinfleis opened this issue Dec 31, 2023 · 3 comments
Open

API: how to expose coverage_union? #3117

martinfleis opened this issue Dec 31, 2023 · 3 comments

Comments

@martinfleis
Copy link
Member

I am wondering how (and if) do we want to expose shapely.coverage_union because the way it behaves on shapely side is super weird. The function assumes two arrays, that are of the same length but does not return a row-wise union as shapely.union does but a single (Multi)Polygon as a union of all of it. Geometries from array a, geometries from array b, all merged to a single geometry.

What I don't understand is the use case for such a function. The assumption here is that polygons form a coverage. Then when would I use coverage_union with its expectation of aligned arrays over coverage_union_all on concatenated arrays? The outcome is exactly the same? Why does shapely.coverage_union exists, especially since it is implemented as return coverage_union_all([a, b], **kwargs)?

My suggestion is to not expose it on geopandas side and expose only coverage_union_all within union_all via a keyword.

@theroggy
Copy link
Member

theroggy commented Jan 4, 2024

I would guess it was just incorrectly implemented in shapely (pygeos) and nobody ever noticed?

@jorisvandenbossche
Copy link
Member

Indeed, I think the way the two arrays in shapely.coverage_union are put together should be transposed to work as expected.

Anyway, for the topic of "unary_union", the relevant function is shapely.coverage_union_all (as equivalent of shapely.union_all, t which shapely.unary_union is aliased), and that one is working correctly.

I agree that for this use case, we can add some "method" keyword to union_all() (in a next shapely release, there might be another method once GEOSDisjointSubsetUnion (GEOS 3.12+) is exposed)


I think there theoretically can be a use case for the element-wise coverage_union, although that's clearly not implemented correctly right now. But let's discuss that on the shapely side. And then if that would be fixed (and not removed), we can still discuss whether we also want to add a coverage_union() method here.

@jorisvandenbossche
Copy link
Member

Opened shapely/shapely#1963 on the shapely side

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

No branches or pull requests

3 participants