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 a fallback to geometry for FeatureCollections #152

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

asinghvi17
Copy link
Member

@asinghvi17 asinghvi17 commented Aug 8, 2024

Fixes #151

What do people think of this?

This enables:

tab = Shapefile.Table(...)
shapes = GI.geometry(tab)

# or, and more importantly,

tab = GeoJSON.read(...)
shapes = GI.geometry(tab)

This can always be customized for individual tables for efficiency's sake, but from a UI perspective I think this is a solid improvement over what we have now.

…azy iterator over all geometry in the collection
@asinghvi17 asinghvi17 marked this pull request as draft August 8, 2024 16:22
@asinghvi17 asinghvi17 changed the title [WIP] Add a fallback to geometry for FeatureCollections Add a fallback to geometry for FeatureCollections Aug 8, 2024
@rafaqz
Copy link
Member

rafaqz commented Aug 8, 2024

Seems sensible to me. Maybe a little confusing that its not getgeom and also allows getgeom(fc, i) ?

@asinghvi17
Copy link
Member Author

asinghvi17 commented Aug 8, 2024

getgeom doesn't work for Features by definition though, we may want to change the API spec in some theoretical 2.0 to make that work? I guess the idea back then was probably a separation of interests between Features and Geometries (see e.g trait vs geomtrait)...

I could definitely allow geometry(fc, i) as well though.

@rafaqz
Copy link
Member

rafaqz commented Aug 8, 2024

I'm not sure what you mean "by definition"... we are talking about FeatureCollection not Feature and its currently just undefined.

We could equally say geometry doesn't work for FeatureCollection "by definition".

The reason to use getgeom is that it always works on a list of geometries, and can choose an indexed geometry, while geometry currently just returns a single geometry

(best not to read to much "I guess the idea back then..." with features, they just were not given much thought, we nearly didn't include them at all)

@rafaqz
Copy link
Member

rafaqz commented Aug 10, 2024

Just hit some code needing this pretty badly...

But I do think getgeom is a more suitable method for getting an iterable of geometries or a single geometry by index from from a feature collection - as it matches the same behavior and return values as for a geometry collection.

@evetion any thoughts on this?

@evetion
Copy link
Member

evetion commented Aug 11, 2024

So we shot ourselves in the foot with the geometry name for Features (to be compatible with pre v1 of GeoInterface). The Feature(Collection) part of the spec has always a been underdeveloped.

Current

properties(::Feature)
geometry(::Feature)

getfeature(::FeatureCollection) and getfeature(::FeatureCollection, i)
geometrycolumns(::FeatureCollection) = (:geometry,) can be multiple!

Suggestion

  • deprecate geometry in favor of getgeom (just rename) and think about getprop? The naming seems more consistent (get prefix is used for geometries and for featurecollections).
  • getgeom on fc is just a getgeom(getfeature) iterator
  • think well about handling multiple geometries using geometrycolumns. I think the spec forbids a feature from having multiple geometries, but given we equate a row in a table with a feature, we implicitly allow it.

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