georust / geos

Rust bindings for GEOS
https://docs.rs/geos/
MIT License
122 stars 41 forks source link

Implement `copyFromBuffer`, `copyFromArrays`, `copyToBuffer` and `copyToArrays` #121

Closed kylebarron closed 1 year ago

kylebarron commented 1 year ago

I'm very inexperienced at C/rust ffi, so happy to get some feedback before adding all four functions and tests.

Closes https://github.com/georust/geos/issues/117

kylebarron commented 1 year ago

Is this crate concerned at all with m values? If they aren't supported anywhere else, should I remove them from these functions?

kylebarron commented 1 year ago

I added a couple more doctests. I think the new_from_buffer function signature should be simplified still, especially because it currently lets you pass in size: 3 but has_z: false and has_m: false. If we assume we never have m values then that makes this a little simpler.

The other thing to note is in get_as_buffer/copyToBuffer. It appears that GEOS always stores coordinates internally as 3 dimensions, so copyToBuffer will fill in NaN values for z coords. Should the API here handle repacking into a 2d buffer? For my case that's unnecessary because I'll be repacking again into arrow memory anyways

GuillaumeGomez commented 1 year ago

I added a couple more doctests. I think the new_from_buffer function signature should be simplified still, especially because it currently lets you pass in size: 3 but has_z: false and has_m: false. If we assume we never have m values then that makes this a little simpler.

size is independent from the m as far as I can see: https://github.com/libgeos/geos/blob/0a55b3af552470754893485b51407cc0219dbaae/capi/geos_ts_c.cpp#L2435-L2443

kylebarron commented 1 year ago

size is independent from the m as far as I can see: libgeos/geos@0a55b3a/capi/geos_ts_c.cpp#L2435-L2443

That's so interesting; I should've known to look at the source! So passing in a buffer of XY is not a straight memcpy

kylebarron commented 1 year ago

I added a couple more doctests. I think the new_from_buffer function signature should be simplified still, especially because it currently lets you pass in size: 3 but has_z: false and has_m: false. If we assume we never have m values then that makes this a little simpler.

size is independent from the m as far as I can see: libgeos/geos@0a55b3a/capi/geos_ts_c.cpp#L2435-L2443

Oops I meant to write dims, not size. size is the number of coordinates in the array; dims is the number of dimensions; we should be able to infer dims from has_z and has_m

GuillaumeGomez commented 1 year ago

Time for the last part of the PR: fixing CI errors. :)

Don't worry about errors from valgrind though. Also, don't forget to rebase on master.

kylebarron commented 1 year ago

I updated with better docstrings, a bug fixed in as_buffer, better handling of m values.

GuillaumeGomez commented 1 year ago

Clippy is still unhappy.

kylebarron commented 1 year ago

I put a feature flag on the type def. Now I'm getting a pass locally with cargo clippy -- -D warnings on Clippy 0.1.67

GuillaumeGomez commented 1 year ago

Clippy is still failing for the same reason.

kylebarron commented 1 year ago

Clippy is still failing for the same reason.

It is? The Clippy output is different in my last two commits. The first time it had a warning related to my changes; now all the clippy warnings are unrelated to my changes. I can change the lifetime syntax there if you'd like

GuillaumeGomez commented 1 year ago

Oh sorry, didn't pay attention. I opened https://github.com/georust/geos/pull/122 to fix it. In the meantime, please squash your commits.

kylebarron commented 1 year ago

I guess this is why on my own repos I usually let github squash my commits for me, because I'm inept at git 😂

kylebarron commented 1 year ago

I guess there's no way for me to re-open this PR, even if I push a new commit to the branch now; sorry about that! I'll have to open a new PR

GuillaumeGomez commented 1 year ago

I wrote a tuto a long time ago about it here. But the other easy way with small PRs is to simply remove all changes and then do them again and commit cleanly. Sometimes, the "not so great way" works well enough.

GuillaumeGomez commented 1 year ago

Nah, reopening in progress. :p

kylebarron commented 1 year ago

My issue was I followed the second suggestion in https://stackoverflow.com/a/25357146 and ran

git reset --soft $(git merge-base master HEAD)

but then ran git push -f, forgetting to commit again 😆

Nah, reopening in progress. :p

thanks! didn't know you could do that

GuillaumeGomez commented 1 year ago

thanks! didn't know you could do that

Me neither until we ended up in this situation. ;)

GuillaumeGomez commented 1 year ago

Thanks!

kylebarron commented 1 year ago

thanks for your help!