feat(expr-geo)!: XYZM passthrough for simplify and convex_hull ops - #1462
feat(expr-geo)!: XYZM passthrough for simplify and convex_hull ops#1462willcohen wants to merge 5 commits into
Conversation
|
That patch isn't working. Need to rework to go off branch for WIP. |
|
I don't think I'm going to get CI to be able to run off of an unmerged branch for geo, but this would still be useful to get a first set of review on! (the tests pass locally) |
|
I think you need to check cargo.lock to see if there are multiple versions of Or, in particular, it looks like you correctly pinned the top-level |
c8a1b2b to
6cc78af
Compare
|
Still a draft pending the upstream PR to georust/geo. Thank you @kylebarron for helping me navigate CI. |
40160d8 to
ac7722f
Compare
54574cf to
2e3a99e
Compare
|
Sorry for the churn. I'm still getting the hang of clippy (it's teaching me a lot, just not invoking it locally the right way to line up with CI) and rust release cadence. Reworked now that georust/geo#1538 has merged: the pin targets an exact rev of geo main, rebased to main, and simplify_vw_preserve now shares one traversal with the other two simplify ops. Also dealt with a few edge cases (unclosed rings, polygon RDP's 4-coord ring minimum, vw at epsilon <= 0), each with a regression test. I think this is ready for review. |
|
One more change: |
|
^ Just a quick review; I haven't made it through the entire PR yet. Some of the (seemingly) clauded code is pretty verbose 😕 |
|
I’ll give this a better rework today on de-verbosity. These comments are very helpful. I still dont have a good spidey sense of when Claude is going full Claude on Rust versus what is idiomatic. I’ll move this back to draft.
|
|
Ok. The insane number of tests is way slimmed down. I'm still not sure the traits are implemented the right way. If you'd like me to consolidate this back into a smaller number of commits, let me know. |
|
Branch reworked and force pushed (each commit now consolidates the changes into single tasks, and isolated the file moves for simplify into their own commit). The actual implementation itself is unchanged from what you last reviewed. Comments are limited now to noting a few quirks relative to geo and the reason for a rustfmt skip. The amount of testing is still a little more intense than otherwise present in the crate, but since this PR is now creating a mechanism to carry coordinates alongside geo, that makes sense to me. Happy to slim the testing apparatus back further, though, if that feels unnecessary. |
|
More cleanup since your review: simpler test assertions, no unsafe blocks left, |
|
Since it's been a while, I squashed the post-review edits into the commits they amended so it's cleaner to re-review each commit -- the branch is five commits again. I also trimmed down the pin in the first commit down, now that geo-types 0.7.20 is out: |
Preserves Z and M through the simplify family (
simplify,simplify_vw,simplify_vw_preserve) andconvex_hull; previously these dropped to XY.Breaking:
convex_hullreturnsArc<dyn GeoArrowArray>instead ofPolygonArray, since each row's hull can carry a different dimension.Needs the index APIs from georust/geo#1538 (merged, unreleased), so
geoispinned to a rev of geo main until geo 0.34;
geo-types/geo-traitsnow comefrom crates.io (geo-types 0.7.16 → 0.7.20). When geo cuts a new release, this
can replace the git rev with the release on crates.io.