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.

Sign up for GitHub

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

Jump to bottom

ENH: Add parameters method and keep_collapsed to make_valid + improve doc #1941

Merged
merged 24 commits into from Feb 12, 2024
Merged

ENH: Add parameters method and keep_collapsed to make_valid + improve doc #1941

merged 24 commits into from Feb 12, 2024

Conversation

theroggy
Copy link
Contributor

@theroggy theroggy commented Nov 27, 2023 •
edited

resolves #1415

reference #1882

@theroggy theroggy changed the title Add parameters method and keep_collapsed to make_valid ENH: Add parameters method and keep_collapsed to make_valid Nov 27, 2023
@theroggy theroggy marked this pull request as draft November 29, 2023 20:13
brendan-ward
brendan-ward reviewed Nov 29, 2023
View reviewed changes
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.

A couple suggestions to get the tests running.

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

theroggy commented Nov 29, 2023 •
edited

Odd that the runtime tests behave differently on windows vs linux. Thanks for the feedback!

I made the proposed changes + added an extra check on GEOS version that should fix a compile error with older geos versions.

So, ready for another try...

@theroggy theroggy marked this pull request as ready for review November 29, 2023 20:54
@theroggy
Copy link
Contributor Author

BTW... something I wanted to mention as I was a bit surprised by it... I don't very often work with lines, but apparently this line is valid: LineString([(0, 0), (1, 1), (1, 2), (1, 1), (0, 0)])... I didn't expect that.

@coveralls
Copy link

coveralls commented Nov 29, 2023 •
edited

Pull Request Test Coverage Report for Build 7481342154

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 88.068%
Totals Coverage Status
Change from base Build 7408068634: 0.4%
Covered Lines: 2598
Relevant Lines: 2950
💛 - Coveralls

@brendan-ward
Copy link
Collaborator

LineString([(0, 0), (1, 1), (1, 2), (1, 1), (0, 0)])
is valid as a LineString but not as a LinearRing.

brendan-ward
brendan-ward reviewed Nov 30, 2023
View reviewed changes
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 @theroggy ! Just a few more suggestions...

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

Suggestions applied...

brendan-ward
brendan-ward approved these changes Dec 4, 2023
View reviewed changes
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 @theroggy ! 🚀

@theroggy
Copy link
Contributor Author

theroggy commented Dec 4, 2023

@brendan-ward can you start the CI tests again on this PR.

jorisvandenbossche
jorisvandenbossche reviewed Dec 4, 2023
View reviewed changes
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.

Thanks a lot for working on this!

shapely/constructive.py Outdated Show resolved Hide resolved
shapely/constructive.py Outdated Show resolved Hide resolved
shapely/constructive.py Outdated Show resolved Hide resolved
jorisvandenbossche
jorisvandenbossche reviewed Jan 10, 2024
View reviewed changes
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.

There is a small linting error to fix, see the CI build

shapely/constructive.py Outdated Show resolved Hide resolved
@theroggy
Copy link
Contributor Author

theroggy commented Jan 10, 2024 •
edited

@jorisvandenbossche feedback applied...

@theroggy theroggy changed the title ENH: Add parameters method and keep_collapsed to make_valid ENH: Add parameters method and keep_collapsed to make_valid + improve doc Jan 16, 2024
jorisvandenbossche
jorisvandenbossche approved these changes Feb 12, 2024
View reviewed changes
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.

Thanks!

@jorisvandenbossche jorisvandenbossche merged commit 84c5ad4 into shapely:main Feb 12, 2024
24 checks passed
@theroggy theroggy deleted the Add-parameters-`method`-and-`keep_collapsed`-to-`make_valid` branch February 12, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

@brendan-ward brendan-ward brendan-ward approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ENH: expose MakeValid parameters, introduced with GEOS 3.10.0
4 participants
@theroggy @coveralls @brendan-ward @jorisvandenbossche