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 geo-traits reader implementation #38

Merged
merged 1 commit into from
Dec 22, 2024

Conversation

kylebarron
Copy link
Contributor

We have a new core geo crate: geo-traits! This enables zero-copy vector data interchange throughout the GeoRust ecosystem.

This means that any library out there that accepts geo-traits input will just work with shapefile geometries.

This PR implements these traits on new-types around shapefile geometry types.

Change list

  • Traits are implemented on existing types where possible. E.g. traits are implemented on Point{M,Z}, MultiPoint{M, Z}, and PolyLine{M, Z}.
  • A new MultiPolygon type is created to wrap a shapefile::Polygon. This is a necessity because geo_traits::PolygonTrait makes a distinction between interior and exterior rings. So a bit of pre-processing is needed to ensure that consumers get constant-time coordinate access.
  • A few new public types are necessitated by being the iteration objects from MultiPolygon and Polyline.

Questions

  • How to handle the dimension for Z geometry types, since we don't know up front whether they'll have M values or not.

Copy link
Owner

@tmontaigu tmontaigu left a comment

Choose a reason for hiding this comment

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

I don"t know if checking the NO_DATA for M values is worth doing, maybe we could let that thing to the user (but then it's not clear if that's a good idea)

src/geo_traits_impl.rs Outdated Show resolved Hide resolved
src/geo_traits_impl.rs Outdated Show resolved Hide resolved
src/geo_traits_impl.rs Outdated Show resolved Hide resolved
@kylebarron
Copy link
Contributor Author

I don"t know if checking the NO_DATA for M values is worth doing, maybe we could let that thing to the user (but then it's not clear if that's a good idea)

I think the semantics here are that data producers are expected to produce valid data, so I think we should keep the check for M values.

Cargo.toml Outdated Show resolved Hide resolved
@kylebarron
Copy link
Contributor Author

geo-traits 0.2 has been released, and I updated this PR to use that.

Comment on lines 108 to 115
// TODO: is it valid to have NO_DATA here? I would've expected that a `PointM` would
// _always_ have an M coordinate.
// if self.m <= NO_DATA {
// geo_traits::Dimensions::Xy
// } else {
// geo_traits::Dimensions::Xym
// }
geo_traits::Dimensions::Xym
Copy link
Owner

Choose a reason for hiding this comment

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

This is not consistent with the import for &PointM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this to check if self.m <= NO_DATA, but I'm not sure if that's the correct behavior. It seems like a PointM should always have an m value, because if it didn't have one, it could just be a Point, right?

src/geo_traits_impl.rs Outdated Show resolved Hide resolved
src/geo_traits_impl.rs Outdated Show resolved Hide resolved
src/geo_traits_impl.rs Outdated Show resolved Hide resolved
src/geo_traits_impl.rs Outdated Show resolved Hide resolved
@tmontaigu tmontaigu force-pushed the kyle/geo-traits-impl branch from f2ddf78 to ab92ab0 Compare December 22, 2024 21:53
@tmontaigu tmontaigu merged commit ab92ab0 into tmontaigu:master Dec 22, 2024
@tmontaigu
Copy link
Owner

Thanks for this

@kylebarron
Copy link
Contributor Author

While switching to use this implementation in geoarrow-rs, I realized that I couldn't fully use it directly because the MultiPolygon and MultiPolygonZ types from geo_traits_impl aren't publicly exported. We aren't able to directly implement the traits on the core Polygon type because the trait impl needs to know where the interior and exterior rings start and stop, and we need a processing step to find that out.

Ideally I'd prefer a method on the already-public Polygon struct like as_trait that returns impl PolygonTrait<T = f64>, but that doesn't work because the Polygon is generic, so we wouldn't know which geo_traits MultiPolygon struct to convert it to.

So perhaps the best solution is to publicly export the MultiPolygon structs from geo_traits_impl and document when to use them. There are From<> impls on MultiPolygon and MultiPolygonZ, but the types aren't public to use that impl.

@tmontaigu
Copy link
Owner

Ideally I'd prefer a method on the already-public Polygon struct like as_trait that returns impl PolygonTrait<T = f64>, but that doesn't work because the Polygon is generic, so we wouldn't know which geo_traits MultiPolygon struct to convert it to.

Not sure I fully understand this be cause we could have

impl Polygon {
  pub fn as_geo_trait(self) -> MultiPolygon;
}

impl PolygonM {
  pub fn as_geo_trait(self) -> MultiPolygonM;
}

impl PolygonZ {
  pub fn as_geo_trait(self) -> MultiPolygonZ;
}

Also i'm tempted to make them be methods that returns results instead of panicking

And I still think we need to pub expect the wrapper types with have for the geo_traits as it might prevent the user
from doing some things (just like you have encountered)

@kylebarron
Copy link
Contributor Author

Ideally I'd prefer a method on the already-public Polygon struct like as_trait that returns impl PolygonTrait<T = f64>, but that doesn't work because the Polygon is generic, so we wouldn't know which geo_traits MultiPolygon struct to convert it to.

Not sure I fully understand this be cause we could have

impl Polygon {
  pub fn as_geo_trait(self) -> MultiPolygon;
}

impl PolygonM {
  pub fn as_geo_trait(self) -> MultiPolygonM;
}

impl PolygonZ {
  pub fn as_geo_trait(self) -> MultiPolygonZ;
}

Oh, maybe you're right; I'm not sure what I was thinking.

Also i'm tempted to make them be methods that returns results instead of panicking

Where do they currently panic?

And I still think we need to pub expect the wrapper types with have for the geo_traits as it might prevent the user from doing some things (just like you have encountered)

In general, as long as the user can access something that implements the desired "top-level" trait, I don't think we need to export the concrete structs. In the same way as the WKB crate (I wrote) returns an opaque impl GeometryTrait<T = f64>. The user doesn't need to have access to the concrete structs.

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

Successfully merging this pull request may close these issues.

2 participants