Converter functions for geo-traits to geo-types

[kylebarron]

• 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.

---

Closes #1253

[michaelkirk]

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.

[kylebarron]

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? 🤷‍♂️

[frewsxcv]

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

[kylebarron]

I added panicking and non-panicking variants

[kylebarron]

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.

[michaelkirk]

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

[michaelkirk]

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).

[kylebarron]

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?

[michaelkirk]

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?)

[michaelkirk]

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.

[kylebarron]

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?

[michaelkirk]

Dealers choice.

[kylebarron]

I switched to traits. Do you like this impl?

[frewsxcv]

Looks good to me!

[kylebarron]

@michaelkirk any thoughts?

[michaelkirk]

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]

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

unavailable

[frewsxcv]

Ugh is main broken again

[urschrei]

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]

@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]

adapt the converter module to the new API.

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

[urschrei]

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]

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.