Converter functions for geo-traits to geo-types by kylebarron · Pull Request #1255 · georust/geo

kylebarron · 2024-11-03T12:50:11Z

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.

michaelkirk · 2024-11-06T18:17:53Z

geo-traits/src/to_geo.rs

+    if let Some(coord) = point.coord() {
+        Point(coord_to_geo(&coord))
+    } else {
+        panic!("converting empty point to geo not implemented")


If we can panic from user input, it should be documented under a # Panics heading.

In general I wish we'd think harder about how to avoid panics. Please read my rant if you haven't already: #1244

In this case, how about returning an Option<Point<T>>. Note it would cascade to the to_geometry function as well.

If you think it's not worth it, then an alternative I'd be willing to move forward with would be to document these panics, and add non-panic versions that return Option: try_point_to_geo.

I agree this panic is unfortunate and not ideal.

In this case, how about returning an Option<Point<T>>. Note it would cascade to the to_geometry function as well.

I'm open to this for to_point but I think it seems rather bad for to_geometry.

Perhaps the best way is to have panicking and non-panicking variants? 🤷‍♂️

Yeah, panicking and try_* non-packing variants seems like a good middleground.

I added panicking and non-panicking variants

kylebarron · 2024-11-06T18:45:12Z

geo-traits/src/to_geo.rs

+/// Convert any coordinate to a [`Coord`].
+///
+/// Only the first two dimensions will be kept.
+pub fn coord_to_geo<T: CoordNum>(coord: &impl CoordTrait<T = T>) -> Coord<T> {


Since these functions are exported from the to_geo mod, should we name it to_coord instead of coord_to_geo?

use geo_traits::to_geo; to_geo::to_coord(...); to_geo::to_point(...);

etc.

Or should that be: to_geo::from_coord(some_coord_trait) 🤯😱💥

To be clear, I don't really know. I do think your shorter name is better than what's there current and unlikely to be confusing in context (assuming the caller keeps the to_geo in scope).

Either way. I don't have a preference. Maybe from_coord is preferable in the hope that some day in the future the traits are stable enough that we could have geo_types::Coord::from_coord?

You could do that today if you introduced a trait:

in geo-traits crate:

impl FromPointTrait { fn from_point(point: impl PointTrait) -> Self; } impl FromPointTrait on geo::Point { ... }

And then also in other crates:

impl FromPointTrait for wkt::Point { ... }

Then you could do things like:

let geo_point = geo::Point::new(1.0, 2.0); let wkt_point = wkt::Point::from_point(geo_point); let back_to_geo_point = geo::Point::from_point(wkt_point);

Is that useful? I dunno. For some formats it'd be a nice conversion muxer between formats. Though I don't know that it would work for all formats where there isn't a "standalone" structure for one geometry (e.g. converting something to a single standalone GeoArrow point might be weird, right?)

I've #1157 (comment) that I don't think the traits should necessarily provide a way to construct new objects. I think that should be implementation defined.

In any case, I'm not advocating for adding a new method to CoordTrait, like you were arguing against in that comment, but rather to add a new trait designating "This thing can be constructed from a impl geo-traits"

I think that's basically the From foil of what you're proposing in To form with ToGeoCoord.

Right, that's an important distinction. Especially because the From could be optionally implemented and isn't a requirement to implement the trait.

Rather I could imagine a trait being defined like

I like that. I'm happy with either merging something like ToGeoCoord or to_coord as you previously phrased.

Happy to do whatever. Any preferences?

Dealers choice.

I switched to traits. Do you like this impl?

Looks good to me!

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

@michaelkirk any thoughts?

michaelkirk · 2024-11-11T21:21:43Z

I'm on holiday for a couple weeks so can't really look closely. The approach looks good though! I'm happy to see it merged if someone else approves.

kylebarron · 2024-11-12T02:03:04Z

I don't think this will merge until Michael removes his request for changes.

unavailable

frewsxcv · 2024-11-12T03:36:26Z

Ugh is main broken again

urschrei · 2024-11-12T10:34:53Z

It's failing on Clippy due to deprecations and a JTS overlay test, both related to i_overlay, which had a release (1.8) yesterday. We were on 1.7.2

urschrei · 2024-11-14T23:58:36Z

@frewsxcv i_overlay 1.8 was breaking (I think intentionally), so we can't merge til we either pin to 1.72, or adapt the converter module to the new API.

kylebarron · 2024-11-15T01:26:01Z

adapt the converter module to the new API.

I don't think anything in this PR touches i_overlay?

urschrei · 2024-11-15T02:05:14Z

adapt the converter module to the new API.

I don't think anything in this PR touches i_overlay?

Sorry, I meant the boolops module that handles interaction with i_overlay!

kylebarron · 2024-12-20T16:27:50Z

In this PR, we had the choice of whether to go with free functions or a trait-based approach and I chose the trait-based approach because it seemed like a cleaner API. Unfortunately, using the trait-based API with GeometryTrait and GeometryCollectionTrait hits this compiler regression: rust-lang/rust#128887, rust-lang/rust#131960

One way to get around that bug is to use the nightly compiler with RUSTFLAGS="-Zinline-mir=no". But that requires the nightly compiler, which is annoying to enforce. So in geoarrow/geoarrow-rs#956 I removed my use of ToGeoGeometry and ToGeoGeometryCollection, replacing them with free functions instead, which allows us to side step the upstream compiler bug and compile normally in release mode.

Closes #716. Ref georust/geo#1255 (comment). Note that this is the same underlying implementation as upstream geo in <georust/geo#1255>. However, the trait-based implementation hits this compiler regression <rust-lang/rust#128887>, <rust-lang/rust#131960>, which prevents from compiling in release mode on a stable Rust version. For some reason, the **function-based implementation** does not hit this regression, and thus allows building geoarrow without using latest nightly and a custom `RUSTFLAGS`. Note that it's only `GeometryTrait` and `GeometryCollectionTrait` that hit this compiler bug. Other traits can use the upstream impls.

Closes geoarrow/geoarrow-rs#716. Ref georust/geo#1255 (comment). Note that this is the same underlying implementation as upstream geo in <georust/geo#1255>. However, the trait-based implementation hits this compiler regression <rust-lang/rust#128887>, <rust-lang/rust#131960>, which prevents from compiling in release mode on a stable Rust version. For some reason, the **function-based implementation** does not hit this regression, and thus allows building geoarrow without using latest nightly and a custom `RUSTFLAGS`. Note that it's only `GeometryTrait` and `GeometryCollectionTrait` that hit this compiler bug. Other traits can use the upstream impls.

Converter functions for geo-traits to geo-types

02bfe52

frewsxcv approved these changes Nov 4, 2024

View reviewed changes

This was referenced Nov 5, 2024

Add benchmarks for copy to geo-types georust/wkb#26

Closed

prepare for geo-traits 0.2.0 release #1263

Merged

michaelkirk previously requested changes Nov 6, 2024

View reviewed changes

kylebarron commented Nov 6, 2024

View reviewed changes

kylebarron and others added 2 commits November 7, 2024 17:21

Switch to traits

b553005

Merge branch 'main' into kyle/traits-to-geo

da03b8b

kylebarron requested a review from michaelkirk November 11, 2024 18:54

frewsxcv approved these changes Nov 11, 2024

View reviewed changes

frewsxcv enabled auto-merge November 11, 2024 21:27

kylebarron mentioned this pull request Nov 12, 2024

Don't panic for conversion of empty points to geo. geoarrow/geoarrow-rs#852

Closed

frewsxcv requested review from michaelkirk and removed request for michaelkirk November 12, 2024 02:06

frewsxcv added this pull request to the merge queue Nov 12, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 12, 2024

kylebarron mentioned this pull request Nov 12, 2024

Remove pyarrow required dependency nmandery/h3ronpy#64

Merged

frewsxcv added this pull request to the merge queue Nov 14, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 14, 2024

Merge branch 'main' into kyle/traits-to-geo

59060fe

frewsxcv enabled auto-merge November 15, 2024 19:27

frewsxcv added this pull request to the merge queue Nov 15, 2024

Merged via the queue into main with commit 896ea04 Nov 15, 2024

frewsxcv deleted the kyle/traits-to-geo branch November 15, 2024 19:33

kylebarron mentioned this pull request Dec 20, 2024

Work around upstream compiler bug geoarrow/geoarrow-rs#956

Merged

kylebarron mentioned this pull request Jul 16, 2025

Compilation error in release mode when using geo_traits::to_geo::ToGeoGeometry #1385

Open

Conversation

kylebarron commented Nov 3, 2024

Uh oh!

michaelkirk Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelkirk Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelkirk Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylebarron commented Nov 11, 2024

Uh oh!

michaelkirk commented Nov 11, 2024

Uh oh!

kylebarron commented Nov 12, 2024

Uh oh!

Uh oh!

frewsxcv commented Nov 12, 2024

Uh oh!

urschrei commented Nov 12, 2024

Uh oh!

Uh oh!

urschrei commented Nov 14, 2024

Uh oh!

kylebarron commented Nov 15, 2024

Uh oh!

urschrei commented Nov 15, 2024

Uh oh!

kylebarron commented Dec 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

michaelkirk Nov 6, 2024 •

edited

Loading

michaelkirk Nov 6, 2024 •

edited

Loading

michaelkirk Nov 6, 2024 •

edited

Loading