Add deprecation warnings for iteration & getitem in multipart geometries#950
Conversation
|
Looking into #952 showed me a couple places where the library itself actually depends on length / iteration over geometries, so there's more to do here than I realized. |
jorisvandenbossche
left a comment
There was a problem hiding this comment.
Thanks for looking into this!!
a couple places where the library itself actually depends on length / iteration over geometries, so there's more to do here than I realized.
Indeed, wherever this pattern is used in Shapely itself, we can already add a .geom to avoid the warning.
The output of the tests can give a good indication where there are still such cases to fix (assuming good coverage of the tests, of course). See eg https://travis-ci.org/github/Toblerity/Shapely/jobs/712405513 which has in the output:
=============================== warnings summary ===============================
tests/test_getitem.py: 3 tests with warnings
tests/test_multilinestring.py: 2 tests with warnings
tests/test_multipoint.py: 1 test with warning
tests/test_multipolygon.py: 1 test with warning
tests/test_shared_paths.py: 1 test with warning
tests/test_split.py: 12 tests with warnings
/home/travis/build/Toblerity/Shapely/shapely/geometry/base.py:902: 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.
...
(sometimes it can help to let the warning error to find all cases where it is being raised)
Does this already add a warning for GeometryCollection as well? (in which case we should add a test for that)
|
Thanks for the review, @jorisvandenbossche! Looks like there's still a lot to do but I've a good start on it now. With regards to adding a test for GeometryCollection, that's another place I need to address, but I'll wait until #951 is merged since it'll touch the same code. As a thought: do we want to deprecate |
Ah, that's a good point, and something we didn't consider yet in the RFC. I agree that kind of follows out of deprecating iteration/getitem, and similarly users can switch (for this PR we can maybe focus on iteration/getitem, since that seems already quite some work anyways?) |
|
Sure, we can address deprecation (or other changes to) |
|
@tomplex do you have some time to update this? |
|
Yes sorry for the stagnation! I'll aim to get more done here this weekend! With the summer fading I've been focused on being outside instead of writing more code after work =) |
|
That's certainly a good focus! ;-) |
|
Hey @jorisvandenbossche, despite my best efforts I've been struggling to find time to work on this, so if you want to add to it, please feel free. |
|
@tomplex I updated this: merged latest master + fixed some remaining usages of getitem in the tests. The full test build now runs without showing any warning. This should be good for a final review now. |
|
Thanks for pushing on this, @jorisvandenbossche. Things have been busy lately and I've been struggling to keep up. Are there any other required changes before this gets merged? |
|
No, I thnk this is ready for merging. I will do a follow-up PR to also look into the |
As per #932, this adds deprecation warnings for
__iter__and__getitem__in multi-part geometries.Looks like
stacklevel=1gives a message like:which feels right to me.