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 .geoms to all shapes #1881

Open
adishavit opened this issue Sep 3, 2023 · 7 comments · May be fixed by #1965
Open

Add .geoms to all shapes #1881

adishavit opened this issue Sep 3, 2023 · 7 comments · May be fixed by #1965
Labels

Comments

@adishavit
Copy link
Contributor

Multi part geometries have a .geoms sequence of all the parts.
I suggest adding a .geoms to the single part geometries too.
It will return just the shape itself but make usability much more consistent without the need to differentiate.
Today, I actually have a wrapper to allow mw to work consistently.

It should be almost trivial to add, be efficient (no copies) and would not break existing code.
WDYT?

@caspervdw
Copy link
Collaborator

Hi @adishavit , thanks for your suggestion.

Personally I think it is incorrect conceptually to give a simple geometry a “.geoms” attribute.

@adishavit
Copy link
Contributor Author

Many Shapely operation have polymorphic return types: that is, the type of the return value may be different depending on the input (buffer, union, intersection etc.) and may also be different from the input type ofc.

The fact that such functions (e.g. .buffer()) take a shape (e.g. Polygon) and may return either a shape, "simple geometry", (Polygon) or a "collection" (MultiPolygon) as valid return types, already blurs the line between what is a so-called "collection" and what is not.

Adding .geoms to all geometries allows us to write type-agnostic code: everything provides a shape iterator interface.
This simplifies code, making it less verbose and also less error prone.

My own all_poly_iter() function (and similar related functions) are in frequent use on my team and make the code much easier to reason about with less need for verbose special case handling.

Nevertheless, this is an opt-in feature, so if one does not want to use it, they are welcome to avoid it.

@caspervdw
Copy link
Collaborator

I can see your argument and I tend towards approving this change. If your team uses this frequently then it probably has more use cases too. But I'd like to hear @sgillies 's opinion about this as well.

@martinfleis
Copy link
Contributor

Just for some context, geopandas.GeoSeries has a .geometry property that returns self, to allow obj.geometry to return a series no matter if obj is a GeoDataFrame or GeoSeries. It is a similar logic as proposed here.

@sgillies
Copy link
Contributor

sgillies commented Oct 4, 2023

@adishavit @caspervdw In the past I've been interested in eliminating the distinction between a Polygon and a MultiPolygon with one part, but I couldn't find anybody else who cared. I suspect some algorithms are much more efficient if you know that the input is a single valid polygon and not several polygons. But I don't see harm in generalizing geoms to single part geometry types. If I were do it all over again, I might name the property parts, but geoms is fine. I think this suggestion is a good example of defining errors out of existence (as John Ousterhout would say).

@caspervdw
Copy link
Collaborator

Ok let’s do it! @adishavit do you have the time to do the PR?

@Rodrigodd
Copy link

Also looking forward to this! I don't like how methods like union_all can return many different types, and I need to check each one when parsing them.

An alternative would be to add a constructor for GeometryCollection that receives any geometry. It would also be cool to have one for MultiPolygon that receives a MultiPolygon or Polygon, one for MultiLineString that receives MultiLineString or LineString, etc.

Ideally, I would prefer that union_all, etc., always return the same type, but that would be a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants