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

Bugfix multipolygon treatment and adapt to latest Shapely #14

Merged
merged 6 commits into from
Jan 23, 2023

Conversation

adybbroe
Copy link
Owner

@adybbroe adybbroe commented Jan 9, 2023

This PR mainly fixes a problem in the treatment of multi polygons for newer shapely versions.
For a while this deprecation warning has been seen.

ShapelyDeprecationWarning: Iteration over multi-part geometries is deprecated and will be removed in Shapely 2.0. Use the `geoms` property to access the constituent parts of a multi-part geometry.

Now with shapely 2.0 it results in a TypeError and a failure.

  • Closes #xxxx
  • Tests added
  • Tests passed: Passes pytest
  • Passes flake8
  • Fully documented

Adam.Dybbroe added 4 commits January 9, 2023 14:41
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
…ions

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
…onstituent parts of a multi-part geometry.

From 2.0 and onwards the previous code would result in a TypeError

ShapelyDeprecationWarning: Iteration over multi-part geometries is deprecated and will be removed in Shapely 2.0. Use the `geoms` property to access the constituent parts of a multi-part geometry.
@adybbroe adybbroe requested a review from mraspaud January 9, 2023 15:08
@adybbroe adybbroe self-assigned this Jan 9, 2023
@adybbroe adybbroe added bug Something isn't working enhancement New feature or request labels Jan 9, 2023
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@coveralls
Copy link

coveralls commented Jan 10, 2023

Coverage Status

Coverage: 78.9% (+1.8%) from 77.06% when pulling c4b0a7e on bugfix-multipolygon into 4f234b8 on main.

Copy link
Collaborator

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Just one refactor proposition, otherwise lgtm

Comment on lines 329 to 337
shape = geometry.geoms[0]
pth = Path(shape.exterior.coords)
mask = pth.contains_points(points)
for shape in geometry.geoms[start_geom_index:]:

constituent_part = geometry.geoms[start_geom_index:]
for shape in constituent_part.geoms:
pth = Path(shape.exterior.coords)
mask = np.logical_or(mask, pth.contains_points(points))

Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks very similar to get_maks_from_mulitpolygon, can it be factorized?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, true. I think this should be done okay now, and at least one test is covering this

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@codecov-commenter
Copy link

Codecov Report

Merging #14 (c4b0a7e) into main (4f234b8) will increase coverage by 1.71%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
+ Coverage   75.73%   77.45%   +1.71%     
==========================================
  Files          20       20              
  Lines        1966     2000      +34     
  Branches      193      192       -1     
==========================================
+ Hits         1489     1549      +60     
+ Misses        451      422      -29     
- Partials       26       29       +3     
Flag Coverage Δ
unittests 77.45% <100.00%> (+1.71%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
activefires_pp/geometries_from_shapefiles.py 86.36% <100.00%> (+5.41%) ⬆️
activefires_pp/post_processing.py 56.02% <100.00%> (+5.63%) ⬆️
activefires_pp/tests/test_shapefile_geometries.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM

@mraspaud mraspaud merged commit 46b894f into main Jan 23, 2023
@mraspaud mraspaud deleted the bugfix-multipolygon branch January 23, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants