Make HasKernel public or introduce geo::Float trait? #577
Comments
|
I'm in favour of the new public geo::Float trait approach. We can clearly and succinctly document what it's for and what it does in a few lines, and spare downstream users the (entirely necessary internal) complexity of HasKernel IMO. @rmanoka what do you think? |
|
TL;DR: I suggest we do both: create a "alias" trait Comp. geom. algorithms involve two concepts: constructions (such as finding mid point of a line segment), and predicate (such as checking if point lies on / above / below a line segment). In general, constructions are not exact unless we use very complicated exact-arithmetic types (eg. mid point of a line-segment with any finite precision coords. like Our The technicality of I do agree that we should improve the docs for |
That's a good point! Though making a
To be clear, I think the Only based on the name, I'm hoping non-experts will have a better intuition that "The thing you're probably looking for is At the point people want to do geometry with exotic numeric types, then it's more reasonable for us to expect them to understand exactly what the So, to recap, I've been persuaded by @rmanoka that we should both add a public |
To address this directly and reiterate: my concern was not that HasKernel was unnecessarily complicated (though better docs are always welcome!), but that I'm hoping to avoid exposing people to it directly unless they are doing stuff with exotic number types. |
I think this was my doing, and just to reiterate what @michaelkirk said: the HasKernel mechanism is an excellent design and I'm delighted with its implementation! The fact that it's complex is inherent to its intended aim, and I think I speak for everyone when I say that it's solved the problem very well. |
583: Allow external crates to implement generic float methods for Geometry<T> r=rmanoka,urschrei a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Solves #577 (see discussion there for context) You can see it in use here: https://github.com/urschrei/polylabel-rs/pull/6/files Co-authored-by: Michael Kirk <[email protected]>
|
This was done in #583 |
602: +Debug for CoordinateType r=frewsxcv a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [TODO] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- This is intended to address #597 I took a broader approach, and wonder if people are interested in it. The tldr is: ### geo-types - deprecate `CoordinateType`; (now includes `std::fmt::Debug`) - Add `trait CoordNum: CoordinateType;` - Add `trait CoordFloat: CoordNum + num_traits::Float` ### geo Add `trait GeoNum: CoordNum + HasKernel` Add `trait GeoFloat: CoordFloat + HasKernel` **downstream adoption** If your crate interops with geo-types, you may need to make changes like this: ``` pub fn foo<T>(line_string: &geo_types::LineString<T>) where - T: num_traits::Float, + T: geo_types::CoordFloat, { ``` or this (this one is actually only a deprecation, so you could leave it as is for now, but eventually you'll need to make the change): ``` pub fn foo<T>(line_string: &geo_types::LineString<T>) where - T: geo_types::CoordinateType + T: geo_types::CoordNum { ``` **alternative considered** If I went with the minimal approach to adding Debug trait, changes would instead need to look like this: This one, for non-floats, could remain untouched: ``` pub fn foo<T>(line_string: &geo_types::LineString<T>) where T: geo_types::CoordinateType { ``` But anything restricted to floats would need to explicitly include Debug ``` pub fn foo<T>(line_string: &geo_types::LineString<T>) where - T: num_traits::Float, + T: num_traits::Float + std::fmt::Debug, { ``` **motivation:** I initially completed #597 with the most conservative approach, achieved in the first two commits. The first simply adds Debug to CoordinateType and the second fixes the resulting compiler errors. This is a breaking change for anyone constraining generic implementations to just `num_traits::Float`, e.g. like [geojson crate does](https://github.com/georust/geojson/blob/master/src/conversion/to_geo_types.rs#L31) (fix here: https://github.com/georust/geojson/compare/mkirk/geo-types-0.7?expand=1), and polylabel (fix here: https://github.com/urschrei/polylabel-rs/compare/master...michaelkirk:mkirk/geo-types-0.7?expand=1) Compare this to a crate like [geo-booleanop](https://github.com/21re/rust-geo-booleanop/blob/master/lib/src/boolean/helper.rs#L61), for which things "just work" because they targeted CoordinateType. This got me thinking that, similar to #577 (yet unreleased), if we want people to write code targeting geo-types in a generic way, it might be helpful to make it clearer and easier how to do so. So, I've added a CoordFloat and a CoordNum which is basically just a rename of CoordinateType, and applied them liberally within our own code base. I'm hoping this would drive others to reach for these traits rather than specific num_traits when implementing code intended for generic geo-types interop. For crates like geo-booleanop, which targeted CoordinateType, things "just work" after the update, but they'll see a deprecation notice like: ``` $ cargo build warning: use of deprecated trait `geo_types::CoordinateType`: use `CoordFloat` or `CoordNum` instead --> lib/src/boolean/helper.rs:2:29 | 2 | use geo_types::{Coordinate, CoordinateType}; | ^^^^^^^^^^^^^^ | = note: `#[warn(deprecated)]` on by default ``` Which I think is acceptably clear. It's a large number of lines changed change overall, I know! But it's mostly mechanical, and I thought, since we're breaking geo-types, maybe this is a good time to bite the bullet and avoid some future pain. If it's too controversial, as I said, the minimal amount of change we need to enable Debug for CoordinateType is the first three commits (through and including "address fallout of "Add Debug to CoordinateType"). It's still a pretty big change with just that, and note that this will still break the crates I mentioned. I intend to fix the ones I know about, but there's a long tail of crates, which is why I hate breaking geo-types. In the few that I've checked it's been quick to adapt to the change, but it might be worth announcing and letting sit open for a while in case downstream geo-types users want to weigh in. Co-authored-by: Michael Kirk <[email protected]>
A lot of our algorithms depend on Float + HasKernel.
When downstream users want to implement methods that are generic across these same types they are stymied because HasKernel is not public outside of the geo crate.
We could make HasKernel public, and then they could implement for Float+HasKernel, but another option (and my current preference) would be to export a combined trait like:
pub trait geo::Float: geo::CoordinateType + geo::HasKenel + num_traits::Float { }Note, I came across this issue while working on urschrei/polylabel-rs#6 which is a downstream user of geo.
public
HasKernelapproach looks like:geo: https://github.com/georust/geo/compare/mkirk/pub-has-kernel?expand=1
downstream geo user (polylabel): https://github.com/urschrei/polylabel-rs/compare/master...michaelkirk:mkirk/update-geo?expand=1
geo::Floatapproach (my preference) looks like:geo: https://github.com/georust/geo/compare/mkirk/geo-float?expand=1
downstream geo user (polylabel): https://github.com/urschrei/polylabel-rs/compare/master...michaelkirk:mkirk/update-geo-2?expand=1
The reason I prefer the
geo::Floatapproach is:it's a bit tidier from the client perspective (rather than
num_traits::Float+geo::HasKernelthey just havegeo::Float(in fact we could tidy up the geo code base a bit with this)The
HasKerneltype is a bit unintuitive for a casual reader. I think the concept of a Float will be more approachable for people just "trying to make things work".It's very possible there's a better approach, or that I'm missing something in my analysis, so curious what other folks think.
The text was updated successfully, but these errors were encountered: