Raise an error on creation of MultiPoint or LineString with empty components#898
Conversation
…tring with empty components
|
I can't for the life of me figure out why adding 4 (tested) lines of code reduced test coverage by half a percent. |
shapely/geometry/multilinestring.py
Outdated
| geom, ndims = linestring.geos_linestring_from_py(obs[l]) | ||
|
|
||
| if lgeos.GEOSisEmpty(geom): | ||
| raise ValueError("Can't create MultiLineString with empty component") |
There was a problem hiding this comment.
@tomplex what would you think about a more shapely-specific exception here? Like a new shapely.errors.EmptyPartError?
I got this idea from Brett Slatkin: https://books.google.com/books?id=bTUFCAAAQBAJ&pg=PA185&lpg=PA185&dq=slatkin+python+base+exception&source=bl&ots=GahcQjraMD&sig=ACfU3U15-chIf93EZAganyYHVhWGQpI_EA&hl=en&sa=X&ved=2ahUKEwiG_tj4korpAhUOAp0JHUleDVEQ6AEwBXoECAsQAQ#v=onepage&q=slatkin%20python%20base%20exception&f=false
There was a problem hiding this comment.
I love the idea - thanks for sharing that book! It definitely makes sense from a library point of view. You mentioned in #836 you'd rather this be a warning and skip the empty geometry instead of raise-ing. Would you prefer if I re-worked this to do that instead?
There was a problem hiding this comment.
After more thought, I think an exception is the right approach. GEOS is going to crash otherwise, so an exception only helps.
|
While I'm in here, in |
|
Also, I'd be happy to move the relevant tests to |
|
@tomplex let's deal with the existing exceptions in a different PR. Would be a breaking change, a small one. |
|
I think this is functionally "done" but I'm still not sure why coverage was reduced. I'll try and take a closer look this evening. |
|
Don't sweat the little coverage changes, it's not entirely reliable. There's an issue with using assertRegex with python 2.7 though. |
|
Can't believe I missed that...I'll fix that this evening. |
|
🎉 |
Closes #836.
As of Shapely 1.7, asking for the WKT of a MultiPoint or -LineString with an empty component results in a segfault. I figured it made more sense to error in that case and let the user take care of the issue - realistically I can't see much utility in having a MultiPoint or -LineString with an empty component anyway, and I don't think you can create such a geometry through WKT, WKB, or GeoJSON, so it doesn't make sense to allow it through the geometry object constructor, either.