Implement `geo_traits` for `geojson` types. #245

frewsxcv · 2024-10-25T04:25:31Z

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

kylebarron · 2024-10-25T04:57:07Z

Still need to implement for Feature (and FeatureCollection?), but that should be easy.

I wouldn't necessarily expect the traits to be implemented on Feature and FeatureCollection. I think it's fine if they're only implemented on the geometry structs.

kylebarron · 2024-10-27T19:34:40Z

Cargo.toml

@@ -12,11 +12,14 @@ edition = "2018"

 [features]
 default = ["geo-types"]
+geo-traits = ["dep:geo-traits", "dep:bytemuck"]


How do you feel about making this a default dependency? I wouldn't imagine it would add too much to compilation time?

Fine by me! As long as we keep it optional

I'm not sure who's driving this PR at this point, but it seems like this is still outstanding.

src/geo_traits_impl/multi_point.rs

frewsxcv · 2024-10-27T20:05:10Z

src/geo_traits_impl/geometry.rs

+        match self {
+            crate::GeoJson::Feature(f) => f.as_type(),
+            crate::GeoJson::FeatureCollection(_fc) => {
+                unimplemented!("TODO")


TODO here needs to be completed

I assume a FeatureCollection, interpreted as a geometry-trait, would map to a GeometryCollection?

Yep exactly. FeatureCollection.as_type is already implemented to return a GeometryCollection. We just need to add that detail here in GeoJson::FeatureCollection.

src/geo_traits_impl/line_string.rs

michaelkirk · 2024-10-29T17:05:42Z

src/geo_traits_impl/geometry.rs

+        match self {
+            crate::GeoJson::Feature(f) => f.as_type(),
+            crate::GeoJson::FeatureCollection(_fc) => {
+                unimplemented!("TODO")


I assume a FeatureCollection, interpreted as a geometry-trait, would map to a GeometryCollection?

src/geo_traits_impl/line_string.rs

src/geo_traits_impl/geometry_collection.rs

src/geo_traits_impl/multi_line_string.rs

src/geo_traits_impl/geometry_collection.rs

src/geo_traits_impl/line_string.rs

kylebarron · 2024-11-10T15:09:35Z

I don't particularly like the overloading of unknown(0) here, because we're using the same enum variant to say both "we have a known number of dimensions but we don't know their semantic meaning" and "we don't know how many dimensions exist". I think it would be clearer to add a new enum variant to make both meanings clearer.

frewsxcv · 2024-11-10T16:02:30Z

Sounds good to me. Conceptually, would that be much different than updating it to return Option<Dim>? I'm fine with any approach here. Just curious how you're thinking about it.

kylebarron · 2024-11-11T18:49:58Z

Sounds good to me. Conceptually, would that be much different than updating it to return Option<Dim>? I'm fine with any approach here. Just curious how you're thinking about it.

That would be the same as Option<Dim>. I wish consumers didn't have to check if the dimension is None, but perhaps that's the only way to solve some producers like GeoJSON

frewsxcv · 2024-11-11T18:58:05Z

I wish we could bundle the dimension and the non-empty geometry together. Something like Empty | (Dimension, Geometry) rather than (None, Empty) | (Some(Dimension), Geometry).

kylebarron · 2024-11-11T19:10:48Z

I wish we could bundle the dimension and the non-empty geometry together.

I believe in Simple Features they're considered separate. E.g. in WKT you can have any of:

POINT EMPTY
POINT Z EMPTY
POINT M EMPTY
POINT ZM EMPTY

So you can't merge them into a single concept.

frewsxcv · 2024-11-11T20:37:28Z

Ah darn 😞 . You're totally right

kylebarron · 2024-12-12T21:14:21Z

Cargo.toml


 [dependencies]
 serde = { version="~1.0", features = ["derive"] }
 serde_json = "~1.0"
 geo-types = { version = "0.7.13", features = ["serde"], optional = true }
+geo-traits = { version = "0.1.0", optional = true }


@frewsxcv would you be able to update this pr to 0.2.0? Or otherwise I could make a new PR

I won't be able to get to this for the next few days. Feel free to open a new pull request or push directly to this one

michaelkirk · 2024-12-14T00:22:16Z

src/geo_traits_impl/mod.rs

+
+#[derive(bytemuck::TransparentWrapper)]
+#[repr(transparent)]
+pub struct PointType(crate::Position);


I wonder if instead we should migrate all the geojson types from type aliases to actual types - the TransparentWrapper stuff seems expedient, but not flexible.

I don't think we necessarily need to address that in order to merge this PR - the current proposal is all within this new module, so I'm not overly concerned.

I'd be in favor of creating full types. I don't have a preference on whether that should happen in this PR or not. That would obviate the need for using bytemuck here though

Slightly in favor of merging with the current design and iterating with dedicated types after, but I don't feel strongly about that

michaelkirk · 2024-12-18T23:10:20Z

Cargo.toml

@@ -12,11 +12,14 @@ edition = "2018"

 [features]
 default = ["geo-types"]
+geo-traits = ["dep:geo-traits", "dep:bytemuck"]


I'm not sure who's driving this PR at this point, but it seems like this is still outstanding.

michaelkirk · 2024-12-18T23:11:17Z

src/geo_traits_impl/coord.rs

+
+    fn dim(&self) -> Dimensions {
+        match self.0.len() {
+            0 | 1 => panic!("Position must have at least 2 dimensions"),


Consider: { "type": "Point", "coordinates": [] } right?

Granted, to be valid, a geojson MUST have at least two coordinates.

But this library is used to parse input that may be invalid, and I don't think we should be panic'ing in release builds for invalid input. I think this will surprise people in a bad way.

Instead, returning either Dimensions::Unknown(0) or Dimensions::Xy could be reasonable. Or dim could return an Option.

Instead, returning either Dimensions::Unknown(0) or Dimensions::Xy could be reasonable. Or dim could return an Option.

We talked about this above: #245 (comment), #245 (comment), #245 (comment).

And see also georust/geo#1263 (comment).

I'd personally choose a default of Dimensions::Xy. I don't think Dimensions::Unknown(0) should ever be used.

frewsxcv added 4 commits October 25, 2024 00:24

Implement geo_traits for geojson types.

fca637e

unused imports

e6461ef

unneeded doc(hidden)

6f5b520

dry

403a089

frewsxcv marked this pull request as draft October 25, 2024 04:31

comment

0c503b6

make optional; use stable version

b74e52b

frewsxcv marked this pull request as ready for review October 27, 2024 19:25

kylebarron reviewed Oct 27, 2024

View reviewed changes

implement for featurecollection, feature, and geojson

d57de45

frewsxcv commented Oct 27, 2024

View reviewed changes

src/geo_traits_impl/line_string.rs Outdated Show resolved Hide resolved

basic implementation test

466a5cb

michaelkirk reviewed Oct 29, 2024

View reviewed changes

This was referenced Oct 30, 2024

Implement geo-traits for reading from WKT georust/wkt#123

Merged

[traits] rename CoordTrait::nth_unchecked -> nth_or_panic to match conventional semantics georust/geo#1242

Merged

kylebarron reviewed Oct 30, 2024

View reviewed changes

src/geo_traits_impl/line_string.rs Show resolved Hide resolved

kylebarron reviewed Oct 30, 2024

View reviewed changes

src/geo_traits_impl/line_string.rs Outdated Show resolved Hide resolved

frewsxcv added 6 commits November 1, 2024 00:20

format

7bd2f75

implement for &GeoJson

765e37a

remove methods with default impls

40bec4b

dont panic if no geometry

d90e876

intentional unused variable

0c5b469

dont panic if no geometry

743ef4e

frewsxcv mentioned this pull request Nov 11, 2024

Clarify and decide what the unknown dimension does in geo-traits georust/geo#1251

Open

kylebarron reviewed Dec 12, 2024

View reviewed changes

kylebarron added 3 commits December 13, 2024 12:43

Bump to geo-traits 0.2

62d399f

Merge branch 'main' into geo-traits-impl

537991f

fmt and lint

3759c3f

michaelkirk reviewed Dec 14, 2024

View reviewed changes

michaelkirk requested changes Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `geo_traits` for `geojson` types. #245

Implement `geo_traits` for `geojson` types. #245

frewsxcv commented Oct 25, 2024 •

edited

Loading

kylebarron commented Oct 25, 2024

kylebarron Oct 27, 2024

frewsxcv Oct 27, 2024

michaelkirk Dec 18, 2024

frewsxcv Oct 27, 2024

michaelkirk Oct 29, 2024

frewsxcv Oct 29, 2024

michaelkirk Oct 29, 2024

kylebarron commented Nov 10, 2024

frewsxcv commented Nov 10, 2024 •

edited

Loading

kylebarron commented Nov 11, 2024

frewsxcv commented Nov 11, 2024 •

edited

Loading

kylebarron commented Nov 11, 2024

frewsxcv commented Nov 11, 2024

kylebarron Dec 12, 2024

frewsxcv Dec 12, 2024

michaelkirk Dec 14, 2024

kylebarron Dec 16, 2024

frewsxcv Dec 17, 2024

michaelkirk Dec 18, 2024

michaelkirk Dec 18, 2024

kylebarron Dec 19, 2024

Implement geo_traits for geojson types. #245

Are you sure you want to change the base?

Implement geo_traits for geojson types. #245

Conversation

frewsxcv commented Oct 25, 2024 • edited Loading

kylebarron commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylebarron commented Nov 10, 2024

frewsxcv commented Nov 10, 2024 • edited Loading

kylebarron commented Nov 11, 2024

frewsxcv commented Nov 11, 2024 • edited Loading

kylebarron commented Nov 11, 2024

frewsxcv commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement `geo_traits` for `geojson` types. #245

Implement `geo_traits` for `geojson` types. #245

frewsxcv commented Oct 25, 2024 •

edited

Loading

frewsxcv commented Nov 10, 2024 •

edited

Loading

frewsxcv commented Nov 11, 2024 •

edited

Loading