Initial geo-traits implementation #1011

kylebarron · 2023-04-20T23:43:46Z

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.

As discussed on the discord, in #838, and originally in #67, this is an initial bare-bones implementation of geometry access traits for discussion. As @frewsxcv suggested, for now this is a separate crate in the workspace to let us iterate on some ideas.

Future work

Minimize cloning in the geo-types implementation

I got lost in lifetime errors so to get things to compile I added cloning to the geo-types implementations. I.e. for MultiPoint each point is cloned when accessed.

frewsxcv · 2023-04-21T16:41:22Z

geo-traits/src/coord.rs

+    fn y(&self) -> Self::T;
+
+    /// Returns a tuple that contains the x/horizontal & y/vertical component of the coord.
+    fn x_y(&self) -> (Self::T, Self::T);


Do you think it's reasonable to have a default implementation for this so implementers don't need to implement it?

Yeah that makes sense. I figure one possible approach to these traits is to have a lot of default methods that currently live in geo-types, like

geo/geo-types/src/geometry/point.rs

Lines 273 to 275 in c31ac45

pub fn dot(self, other: Self) -> T {

self.x() * other.x() + self.y() * other.y()

}

geo-traits/src/line_string.rs

kylebarron · 2023-04-22T17:30:36Z

geo-traits/Cargo.toml

+readme = "../README.md"
+keywords = ["gis", "geo", "geography", "geospatial"]
+description = "Geospatial traits"
+rust-version = "1.65"


Bumped this to 1.65 for this crate because the traits use GATs, which came in 1.65

michaelkirk · 2023-06-15T17:06:39Z

Thank you for moving the traits discussion forward with some concrete code! I think these proposed trait implementations looks reasonable in the abstract. I'd love to see how some algorithms can be implemented in terms of these traits, and what kind of changes/concessions/benefits arise when rubber meets the road.

I think seeing how they can be used will be a necessary measure of how "good" these trait definitions are.

frewsxcv · 2023-06-16T00:16:37Z

geo-traits/src/geometry.rs

+    MultiPoint(&'a MP),
+    MultiLineString(&'a ML),
+    MultiPolygon(&'a MY),
+    GeometryCollection(&'a GC),


Do you have any thoughts on adding the other types offered in geo-types? Namely Triangle, Line, and Rect

Definitely something to discuss. I didn't implement them yet because I wasn't sure the pros/cons of how hard that might be for some data structures to implement vs the usefulness in adapting for geo algorithms. If a data structure doesn't natively have a Rect type, should it allow a polygon to fill in as a Rect?

geo-traits/src/line_string.rs

frewsxcv · 2023-06-16T00:19:48Z

geo-traits/Cargo.toml

+[features]
+
+[dependencies]
+geo-types = "0.7.9"


What do you think about making geo-types optional? In theory if someone wanted to just implement geo-traits for their own geometry types, they shouldn't need access to geo-types, I think

Hmm actually this depends on CoordTrait. Maybe that should live in this crate someday 🤔

Yeah I think making geo-types optional definitely makes sense but in this initial implementation I made a hard dependency for simplicity

frewsxcv · 2023-06-16T00:26:31Z

geo-traits/src/polygon.rs

+use std::slice::Iter;
+
+pub trait PolygonTrait<'a>: Send + Sync {
+    type ItemType: 'a + LineStringTrait<'a>;


Maybe RingType would be more descriptive? 🤷🏻

Should we change the name ItemType in each trait then? I.e. CoordType in LineStringTrait?

frewsxcv · 2023-06-16T00:27:21Z

geo-traits/src/polygon.rs

+use std::iter::Cloned;
+use std::slice::Iter;
+
+pub trait PolygonTrait<'a>: Send + Sync {


I wonder if we'll want to specify guarantees around winding order for implementors.

I don't know... some stores might not have that information? I think GEOS objects don't have a known winding order?

kylebarron · 2023-06-16T14:08:13Z

I'd love to see how some algorithms can be implemented in terms of these traits, and what kind of changes/concessions/benefits arise when rubber meets the road.

I agree things will become clearer with code implementation. Should I make a new stacked PR to iterate or add edits to this PR?

frewsxcv · 2023-06-16T14:16:45Z

My vote would be to merge as-is, and then iterate on it with subsequent PRs. Then it's easier for others to collaborate too

kylebarron · 2023-06-16T14:51:51Z

Any changes I should make in this PR? Unclear what should be discussed further; maybe add rect/line/triangle?

frewsxcv · 2023-06-16T17:12:21Z

Let's wait to hear what @michaelkirk thinks! If he approves of the plan then yeah we can merge it

michaelkirk · 2023-06-16T17:55:41Z

My concern is that we may be putting the cart before the horse a bit. My understanding of this work is that ultimately the purpose of it is to be able to define algorithms in terms of these traits. geo-types would be one implementor, but people might want to bring their own types which are better suited for their use cases for whatever reasons, and write their own impl geo_traits::* for MyGeometryType - like an Arrow backed geometry. Right?

I don't think we need to come up with a perfect and complete design before merging. I'm saying "show me that the code you want to merge works at all". Yes it compiles, but if the point of these traits is to be able to implement algorithms, then lets see what it looks like to implement an algorithm with them.

My hope is that if this is a reasonably good design for geometry traits, that this will not be an unreasonably hard task. If it turns out to be hard to do, then maybe this is not a good design for a geometry traits and we all will learn something.

michaelkirk · 2023-06-16T18:21:58Z

geo-traits/src/coord.rs

@@ -0,0 +1,64 @@
+use geo_types::{Coord, CoordNum, Point};


🧑‍🚒 hot take: It was a mistake to have geo_types::Coordinate separate from geo_types::Point they are redundant, and I'm guessing this only happened because of ideas copied over from Java where a "light weight" representation required splitting the two concepts.

Rust doesn't have the overhead of "primitive type" vs "object". Regardless of how many methods or whatever we add to it, geo-types::Coordinate<f64> will always be the same size as [f64; 2].

The cost of our current approach is frequent confusion about whether a method can/should accept a Coordinate, a Point, or both.

Maybe geo-traits could be an opportunity to fix that wart. We could delete CoordTrait, and have only PointTrait. Both geo-types::Coordinate and geo_types::Point would implement PointTrait, allowing any methods defined in terms of PointTrait to accept both, and finally resolving the question "Should this be a Point or a Coord?"

This is a half baked armchair suggestion, so no changes demanded, but think about it!

In fact, this might be a relatively nice way to start showing the value of a trait system proposal - by simplifying some of our existing APIs.

Also, no judgement on the original geo_types::Coordinate vs geo_types::Point decision. It's gotten us pretty far, and it seemed totally reasonable to me at first too, until years of stubbing my toe on it.

I'd be fully supportive of making Coord and Point both implement a shared PointTrait! I admit I do get confused when to use coord vs point.

frewsxcv · 2023-06-16T18:27:14Z

@michaelkirk For the purposes of a proof-of-concept, do you see the implementations you desire living in geo or geo-traits?

michaelkirk · 2023-06-16T18:32:31Z

I was assuming our existing algorithms in geo would be modified to work in terms of geo-traits where possible.

Did you have something else in mind?

michaelkirk · 2023-06-16T18:33:46Z

For the purposes of a proof-of-concept

I guess I'm a little uncertain about the usage of "proof of concept". I think that if there is code in georust/geo we should expect that people will use it.

If we want a place to experiment with crazy stuff that we don't want people to use I don't think it should live in this repository.

frewsxcv · 2023-06-16T18:43:38Z

I think that if there is code in georust/geo we should expect that people will use it.

Curious where this concern comes from! And how them using unpublished code would affect the lives of us as maintainers. As long as we are unpaid volunteers supporting their efforts, we don't owe them any guarantees about how we run our work. Unlike many technical decisions, I feel pretty strongly about unpaid labor haha

frewsxcv · 2023-06-16T18:45:51Z

I was assuming our existing algorithms in geo would be modified to work in terms of geo-traits where possible.

Did you have something else in mind?

I ask because if they live in the geo/ directory then that muddies the water in terms of separating our draft code and keeping everything in the geo/ directory deployable.

michaelkirk · 2023-06-16T20:26:35Z

I agree that we should keep geo/ deployable at all times (modulo a version bump in Cargo.toml or whatever).

Can you elaborate on how you'd like to see geo-traits development proceed @frewsxcv? All I can tell is that you seem to not be quite on board with my vision for it. 😅

frewsxcv · 2023-06-16T22:18:07Z

I see us iterating on it in a separate crate directory akin to the other crate directories. If a pull request change to geo-traits is an improvement, we approve it and merge it in. There's no need to increase the barrier to contribution with a separate repository or ensuring the full vision is there. We can do that when we're ready to publish.

b4l · 2023-06-16T22:46:37Z

Alternatively, we could agree to work on a traits branch analogous to a development branch for a while until we figure out if it is the way to go. There we would also have the liberty to directly refactor the existing algorithms and put the code in the right place and have a nice diff.
Mostly working with 3d data nowadays and the stalled effort to introduce another coordinate dimension in geo-types prevents me from using it which I find quite unfortunate.
Just providing options here and hope we can find space to be bold at times and traits may as well offer some possibility to support somehow arbitrary dimension support.

Edit: Also +1 for merging the coord and point traits as long as they don't diverge and what about using std::ops::Index instead of 'point(i: usize)` and the likes to access individual members of multi types?

michaelkirk · 2023-06-17T00:01:17Z

Another concern I have is less objective, but subjectively, I suspect my position as a maintainer gets easier (and thus more delightful) when there are clear delineations between "this code is thought to work" vs. "this code is a prototype, and I'm not sure to what extent it is intended to work". I'm willing to accept that this is an idiosyncrasy founded on my own personal trauma doing open source development on The Internet (🤣😭) and may not be relevant to other people, but it's still my preference.

I see us iterating on it in a separate crate directory akin to the other crate directories.

Ok, I'm with you here, but more specifically, if we merged these trait definitions, what's next? I'm assuming implementing some algorithms with these traits would be the next step (right?).

So how would we go about that? My best guess is you mean we'd be duplicating the existing algorithms from geo into geo-traits and editing that copy to work with geo-traits instead of geo-types? Or did you intend something else?

we could agree to work on a traits branch analogous to a development branch for a while until we figure out if it is the way to go.

(Thanks for chiming in @b4l!)

I think long running branches can be fraught, typically because they can get out of sync, but I do think it's preferable to duplicating/re-implementing the algorithms in geo-traits. At least with a long running branch, version control can give us some hints as to where conflicts arise (and hopefully it won't be that long running anyway, right? 😬). The alternative of duplicating the implementations means we are relying on our own fragile minds to reason about/remember when conflicts occur.

There we would also have the liberty to directly refactor the existing algorithms and put the code in the right place and have a nice diff.

👍👍 Having a diff against the current geo algorithm implementation is the primary thing I'm looking for when trying to evaluate how "good" these trait implementations are.

What do other people think about merging traits work into a traits branch until we're comfortable releasing something usable and relevant to geo?

I'd especially like to hear from @kylebarron, who seems most affected by this decision in the near term.

kylebarron · 2023-06-17T00:55:54Z

I'm happy to have this merge onto a traits branch, and ongoing PRs can have that as the base branch. We could also have an ongoing open draft PR from the traits branch to the main branch to make it easy for people to see and comment on the state of the two branches at large

rmanoka · 2023-06-17T04:13:32Z

This an excellent contribution; thanks @kylebarron ! I'm also quite in favour of exploring the trait-based approach for geo. Specifically w.r. to this PR, I would also prefer we merge this as-is/quickly into a separate branch until we identify a plan to switch to it (either incrementally or in one release). This would help have smaller PRs to help push this out.

Particularly we should try to understand:

Port effort. Is the changes needed per algo. straight-forward or needs knowledge of algo? Do we need to port all algos in one release or can it be done incrementally?
Dependencies. geo-traits abstracts over geo-types, and hence the new dependency chain is: geo -> geo-traits -> geo-types. In the long run, geo won't even directly depend on geo-types. But, are there any dependency issues to understand? Will every breaking bump of geo-traits need bump of geo? This is fine, but would we then have to impl. the algos for different versions of geo-traits via complicated macros?
Performance. Would be good to benchmark some algos on this approach to ensure the compiler understands that we're just wrapping around [f64; 2] or similar.

Pls. add more, and we could track this in the PR that merges the traits branch to main. Reg. @michaelkirk valid concern on the feature branch diverging: since the traits are just wrapping types, at least until we finalize the details of the traits, only changes in geo-types should affect the difference between the branches. So, may be worth the risk?

urschrei · 2023-06-17T08:54:50Z

(apologies for the drive-by comment)
I also prefer the idea of a separate branch for now. Yes, there's some overhead to keeping it in sync, but as has been pointed out it hopefully won't be too long-running.

frewsxcv · 2023-06-19T01:06:50Z

I have a preference for merging into main, but I am also okay with a separate traits branch.

frewsxcv · 2023-06-19T01:08:30Z

I just updated the base branch of this pull request to be a new traits branch. Anyone else have blockers before merging?

kylebarron · 2023-06-19T18:02:17Z

Seems fine on my side; thanks for making the branch!

frewsxcv · 2023-06-21T01:18:21Z

bors r+

🎉 🎉 🎉 🎉 🎉

bors · 2023-06-21T01:25:16Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

ci result

michaelkirk · 2023-06-21T23:17:55Z

🥳
Thanks for your flexibility @frewsxcv.

kylebarron added 8 commits February 8, 2023 23:07

Base traits implementation

91e159b

Implement geometry collection trait

edb6c6c

remove comments

9cdca40

simpler type

bec4fdc

small fixes

f63803e

Merge branch 'main' into kyle/traits-v2

f87562a

Initial geo-traits implementation

2b04f25

remove traits import from geo

6dcba58

frewsxcv reviewed Apr 21, 2023

View reviewed changes

geo-traits/src/line_string.rs Outdated Show resolved Hide resolved

kylebarron added 3 commits April 21, 2023 13:56

Address comments

046704b

make linestring item a coord trait

cf3a51d

Bump geo-traits msrv

6413405

kylebarron commented Apr 22, 2023

View reviewed changes

frewsxcv reviewed Jun 16, 2023

View reviewed changes

kylebarron added 2 commits June 15, 2023 20:51

Merge branch 'main' into kyle/traits-v3

7d639fe

Use ExactSizeIterator

10cdce0

michaelkirk reviewed Jun 16, 2023

View reviewed changes

frewsxcv approved these changes Jun 19, 2023

View reviewed changes

frewsxcv changed the base branch from main to traits June 19, 2023 01:07

bors bot merged commit 1b9557f into georust:traits Jun 21, 2023
1 check passed

frewsxcv mentioned this pull request Jun 21, 2023

Add new geo-traits crate #1019

Closed

2 tasks

frewsxcv mentioned this pull request Jul 29, 2023

Introduce trait for accessing geometries #67

Open

michaelkirk mentioned this pull request Feb 27, 2024

Add geo-traits crate #1157

Open

2 tasks

	pub fn dot(self, other: Self) -> T {
	self.x() * other.x() + self.y() * other.y()
	}

Initial geo-traits implementation #1011

Initial geo-traits implementation #1011

Conversation

kylebarron commented Apr 20, 2023 • edited

Future work

Minimize cloning in the geo-types implementation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk commented Jun 15, 2023

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

Choose a reason for hiding this comment

kylebarron commented Jun 16, 2023

frewsxcv commented Jun 16, 2023

kylebarron commented Jun 16, 2023 • edited

frewsxcv commented Jun 16, 2023

michaelkirk commented Jun 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frewsxcv commented Jun 16, 2023

michaelkirk commented Jun 16, 2023

michaelkirk commented Jun 16, 2023

frewsxcv commented Jun 16, 2023

frewsxcv commented Jun 16, 2023 • edited

michaelkirk commented Jun 16, 2023 • edited

frewsxcv commented Jun 16, 2023

b4l commented Jun 16, 2023 • edited

michaelkirk commented Jun 17, 2023

kylebarron commented Jun 17, 2023

rmanoka commented Jun 17, 2023

urschrei commented Jun 17, 2023

frewsxcv commented Jun 19, 2023

frewsxcv commented Jun 19, 2023

kylebarron commented Jun 19, 2023 • edited

frewsxcv commented Jun 21, 2023

bors bot commented Jun 21, 2023

michaelkirk commented Jun 21, 2023

kylebarron commented Apr 20, 2023 •

edited

kylebarron commented Jun 16, 2023 •

edited

frewsxcv commented Jun 16, 2023 •

edited

michaelkirk commented Jun 16, 2023 •

edited

b4l commented Jun 16, 2023 •

edited

kylebarron commented Jun 19, 2023 •

edited