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

get_coord_seq without extra clone #137

Open
kylebarron opened this issue Sep 30, 2023 · 3 comments · May be fixed by #138
Open

get_coord_seq without extra clone #137

kylebarron opened this issue Sep 30, 2023 · 3 comments · May be fixed by #138

Comments

@kylebarron
Copy link
Member

From the docstring of get_coord_seq:

Note: this clones the underlying CoordSeq to avoid double free (because CoordSeq handles the object ptr and the CoordSeq is still owned by the geos geometry) if this method’s performance becomes a bottleneck, feel free to open an issue, we could skip this clone with cleaner code.

My use case is to bind GEOS algorithms to the GeoArrow memory layout (an efficient geometry layout for arrays of geometries, see geoarrow.org and my WIP rust implementation at https://github.com/geoarrow/geoarrow-rs). My current plan is to always store geometries before and after each operation in GeoArrow memory, and therefore GEOS objects are totally ephemeral during an operation. So the process goes like

  1. Take an array of e.g. polygons
  2. Iterate over the array, converting each into GEOS objects
  3. Apply a GEOS operation, say, buffering
  4. Construct a new GeoArrow array from the polygon outputs

Therefore the IO to and from GEOS objects is really important to me, because it's overhead for every operation on the array.

One option is to improve this get_coord_seq, removing a clone. The other possibility for me is to have something like into_coord_seq or into_inner. Given that I want to consume the GEOS geometry anyways, and only access its coords, this might be easier to implement?

I'd be willing to attempt a PR!

@GuillaumeGomez
Copy link
Member

Both approaches sound good to me but if the into_coord_seq approach is simpler, please go ahead with this one.

Might still be worth to improve get_coord_seq in the future though.

@kylebarron
Copy link
Member Author

Actually thinking about this a little more.. might one way to solve this be to implement a ConstCoordSeq struct just like there's a ConstGeometry?

@kylebarron kylebarron linked a pull request Oct 1, 2023 that will close this issue
@GuillaumeGomez
Copy link
Member

That could work too.

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 a pull request may close this issue.

2 participants