Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP Implement area functions on geometry traits #1021

Closed
wants to merge 15 commits into from

Conversation

kylebarron
Copy link
Collaborator

@kylebarron kylebarron commented Jun 23, 2023

Change list

  • Remove Send + Sync restriction on T on the trait. I don't know whether this restriction makes sense here. I originally added it because rust-postgis included it. But it appeared that CoordFloat doesn't include those trait bounds so it was easier in this context to remove them.
  • Add fully parametrized T: CoordNum on each trait, and define other item types in terms of T. I needed to do this so that some algorithms could enforce CoordFloat and not just CoordNum.
    I'm not sure if type ItemType = LineString<Self::T>; on the impl is necessary instead of type ItemType = LineString<T>;.
  • Implement pure functions for most combinations of geometry type + signed/unsigned area.
  • Drop 1.63 CI from this branch

Questions:

  • Is there a simple way with lifetimes to fix this temporary value dropped while borrowed error? It's on this line. I'm not sure if this is an issue with the trait design or implementation or with my code

@frewsxcv
Copy link
Member

Is there a simple way with lifetimes to fix this temporary value dropped while borrowed error? It's on this line. I'm not sure if this is an issue with the trait design or implementation or with my code

When I run into situations like this, I typically start incrementally loosening lifetime bounds that don't need to be explicitly specified. With these changes, your code compiles: https://gist.github.com/frewsxcv/75e719227b889ee2930ccb59930b26de

x: linestring.coord(i).unwrap().x(),
y: linestring.coord(i).unwrap().y(),
};
c1.x = c1.x - shift.x();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched these to not use -= to avoid having to require the SubAssign type on T. Maybe requiring SubAssign and similar is such a low bar that we should require it?

@kylebarron kylebarron changed the title WIP unsigned area with trait WIP Implement area functions on geometry traits Jun 27, 2023
@kylebarron
Copy link
Collaborator Author

When I run into situations like this, I typically start incrementally loosening lifetime bounds that don't need to be explicitly specified. With these changes, your code compiles: gist.github.com/frewsxcv/75e719227b889ee2930ccb59930b26de

Thanks! That was helpful! I was able to make some progress. But it seems like the trait methods that really need &'a self are the iterators.

So I can make progress by removing 'a everywhere but that forces me to write ugly code and avoid using the iterators, doing stuff like

for polygon_idx in 0..geom.num_polygons() {

Especially with the GeometryCollection and Geometry traits, it gets complex and it's not clear to me we can fully remove the lifetimes.

I tried to implement unsigned_area_geometry which takes anything that implements GeometryTrait, but there as_type requires 'a.

Comment on lines +12 to +15
pub trait GeometryTrait<'a> {
type T: CoordNum;
type Point: 'a + PointTrait<T = Self::T>;
type LineString: 'a + LineStringTrait<'a, T = Self::T>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can now be:

type LineString<'a>: 'a + LineStringTrait<'a, T = Self::T>;

Or something like that

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit to adding <'a> onto the type name as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the lifetime from the trait itself, which means any references to the trait don't need to specify that lifetime. Similar to https://github.com/georust/geo/pull/908/files

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow that is super nice!

Copy link
Collaborator Author

@kylebarron kylebarron Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It ended up being a lot more work than expected but I implemented that change in geoarrow/geoarrow-rs#253 (I'm incubating and testing out trait impls there); I'll make a PR to the traits branch with those updated changes

(I still had lifetime issues connecting to geos but that's life 🤷‍♂️ )

@frewsxcv
Copy link
Member

From what I understand, #1115 is a newer version of this so I'm going to close this one. Feel free to reopen if this is wrong

@frewsxcv frewsxcv closed this Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants