Skip to content

Change simplify() preserve_topology default to True (match the Geometry method)#1392

Merged
jorisvandenbossche merged 1 commit intoshapely:mainfrom
jorisvandenbossche:simplify-default
Jun 7, 2022
Merged

Change simplify() preserve_topology default to True (match the Geometry method)#1392
jorisvandenbossche merged 1 commit intoshapely:mainfrom
jorisvandenbossche:simplify-default

Conversation

@jorisvandenbossche
Copy link
Copy Markdown
Member

xref #1276

The Geometry class' simplify() method has a default of preserve_topology=True, while the vectorized function from pygeos was using True. But I think we have been thinking in the past we should probably move to True anyway (to return valid geometries by default, eg pygeos/pygeos#233 (comment)), and that also ensures that the function and geometry class method use a consistent default.

@coveralls
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 2365845189

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 84.325%

Totals Coverage Status
Change from base Build 2330939066: 0.0%
Covered Lines: 2141
Relevant Lines: 2539

💛 - Coveralls

@mwtoews
Copy link
Copy Markdown
Member

mwtoews commented Jun 7, 2022

I think in the future, there is scope to allow more than two simplify methods, as there are multiple "simplify" algorithms out there (published or coded in some form under a liberal licence). Currently, the preserve_topology bool keyword just enables two methods, but a future-proof way is to have a method keyword instead (base on an enum or str).

@jorisvandenbossche
Copy link
Copy Markdown
Member Author

@mwtoews that's a good point (eg GEOS 3.11 will have a PolygonHullSimplify method, so we can (in another issue) start discussing whether to expose this under simplify with a method keyword, or as a separate function/method).

Regardless of that, I think we still want to switch the default anyway (and we need to keep the existing keyword for some time for backwards compatibility), and so this PR should still be fine as is for now?

Copy link
Copy Markdown
Member

@mwtoews mwtoews 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. We can discuss changes to support more than two algorithms later.

@jorisvandenbossche jorisvandenbossche merged commit a0aae53 into shapely:main Jun 7, 2022
@jorisvandenbossche jorisvandenbossche deleted the simplify-default branch June 7, 2022 18:11
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.

5 participants