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

Add merge overlaps function #50

Merged
merged 2 commits into from
May 15, 2024
Merged

Conversation

lisawink
Copy link
Contributor

@lisawink lisawink commented May 7, 2024

Have added a function merge_overlaps in overlap.py which merges any overlapping polygons sharing an overlap area larger than 'overlap_limit'. Overlapping polygons smaller than 'merge_limit' are always merged, no matter the overlap area. Tests are included in test_overlap.py and examples in overlaps.ipynb

Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 96.96970% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 73.49%. Comparing base (3202732) to head (a4fc4ad).
Report is 1 commits behind head on main.

Files Patch % Lines
geoplanar/overlap.py 96.96% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   70.04%   73.49%   +3.44%     
==========================================
  Files           6        6              
  Lines         217      249      +32     
==========================================
+ Hits          152      183      +31     
- Misses         65       66       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sjsrey sjsrey self-requested a review May 7, 2024 14:20
@sjsrey
Copy link
Owner

sjsrey commented May 9, 2024

Thanks for this, @lisawink

This looks good to me and would add nicely to the package.

The one test is failing with an older version of geopandas. @martinfleis, I'm wondering if we want to consider dropping support/adding a min dep for geopandas, or should we add a skip test if we want to keep testing everything else on the older version?

geoplanar/overlap.py Outdated Show resolved Hide resolved
@martinfleis
Copy link
Collaborator

I am not sure if that last example in the notebook is desired behaviour. It seems weird to see those two large polygons merged together.

@lisawink
Copy link
Contributor Author

I agree it is weird, but because the polygons are merged in order, when the smaller polygon is merged to one of the bigger polygons, this now overlaps with the other big polygon, and so it is all merged together. Any other solution would require trimming, which could be done first with trim_overlaps, and then with merge_overlaps or merge_touching. I think it really depends on the use-case.

@martinfleis
Copy link
Collaborator

That's probably fair.

@sjsrey
Copy link
Owner

sjsrey commented May 15, 2024

I had the same initial reaction as @martinfleis, but then, thinking through the logic, I agree with @lisawink that the results make sense.

This brings up a more general question of how the sequence of operations may result in different outcomes - I think this is a really interesting problem. I think the role of the package would be to handle individual operations correctly and provide the user with the ability to combine them in sequences that make sense for different use cases.

@martinfleis
Copy link
Collaborator

This brings up a more general question of how the sequence of operations may result in different outcomes - I think this is a really interesting problem. I think the role of the package would be to handle individual operations correctly and provide the user with the ability to combine them in sequences that make sense for different use cases.

Fully agree. In our case of building footprints, @lisawink spent some time figuring out the correct order of operations and it does indeed make a difference. But that will be case specific, so I would not impose any sequence within geoplanar.

@martinfleis martinfleis merged commit 5452f88 into sjsrey:main May 15, 2024
10 checks passed
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

3 participants