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: include coverage_union_all in union_all and dissolve #3151

Merged
merged 9 commits into from
Apr 28, 2024

Conversation

martinfleis
Copy link
Member

Xref #2010

I was surprised how faster this actually is. On geodatasets' geoda.south it goes from 121 ms to 4.43 ms. It will likely depend on the data but it is impressive nonetheless.

The formulations in the docstring feel a bit sloppy so I'd appreciate some native speaker to check if it makes sense.

@martinfleis martinfleis added this to the 1.0 milestone Jan 18, 2024
@martinfleis martinfleis changed the title ENH: include coverage_union_all in union_all ENH: include coverage_union_all in union_all and dissolve Jan 19, 2024
Copy link
Member

@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 @martinfleis , this seems like a better way to expose coverage_union_all from Shapely.

A few docstring suggestions...

geopandas/base.py Outdated Show resolved Hide resolved
geopandas/base.py Outdated Show resolved Hide resolved
geopandas/base.py Outdated Show resolved Hide resolved
geopandas/base.py Outdated Show resolved Hide resolved
geopandas/base.py Outdated Show resolved Hide resolved
Parameters
----------
coverage : bool (default False)
Whether to assume non-overlapping coverage and opt-in for the faster
Copy link
Member

Choose a reason for hiding this comment

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

Suggest using the same docstring as below

            If True, will use the faster coverage union to merge geometries.
            If False, the default slower but robust union will be used. 

@martinfleis
Copy link
Member Author

Change the keyword to method with string options to allow GEOSDisjointSubsetUnion to be exposed in future.

martinfleis and others added 2 commits February 6, 2024 12:37
Co-authored-by: Brendan Ward <bcward@astutespruce.com>
Copy link
Member

@m-richards m-richards left a comment

Choose a reason for hiding this comment

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

Thanks martin, looks good to me! Good spot on changing to method= instead of a boolean, that future proofs things nicely.

geopandas/base.py Outdated Show resolved Hide resolved
martinfleis and others added 2 commits February 18, 2024 09:42
Co-authored-by: Matt Richards <45483497+m-richards@users.noreply.github.com>
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Looks good!

CHANGELOG.md Outdated Show resolved Hide resolved
geopandas/base.py Outdated Show resolved Hide resolved
geopandas/geodataframe.py Outdated Show resolved Hide resolved
@jorisvandenbossche jorisvandenbossche merged commit bc71b36 into geopandas:main Apr 28, 2024
20 checks passed
@martinfleis martinfleis deleted the coverage_all branch April 28, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants