Repository navigation
Conversation
`forcexyz` reassigned the captured `z` inside its `apply` closure, so Julia boxed it, and it called `GI.is3d` on the whole input, which fails for vectors of geometries. Checking each point is what the docstring describes. This is ~270x faster on a 10k point polygon. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Counters reassigned inside closures in `_build_a_list` and the `DoubleNaturalTree` edge traversal, and the conditionally reassigned `multipoly_a` in the multipolygon `difference`/`union` methods, were boxed. Use `Ref`s and a `let` block instead. `DoubleNaturalTree` intersection of two 4000-vertex polygons goes from 1.0 ms to 0.34 ms, the default clipping path is ~9% faster, and `difference`/`union` of multipolygons now infer a concrete return type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The state was reassigned inside `map` closures, so it was boxed and every component cost a dynamic dispatch. Reconstructing 25 polygons of 201 points goes from 1.2 ms to 3.2 μs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
|
Thanks, looks awesome! And with the julia 1.12 improvements to allocation elision it should allocate even less than before. Looks like something small broke in the polygon clipping code, I'm setting a bot on it. |
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Harness: codex Model: gpt-6.1-sol
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Towards #371. Scanning the lowered code of all GeometryOps and GeometryOpsCore methods for
Core.Boxfound 9 methods with boxed captured variables (same on Julia 1.10 and 1.13). This fixes 8 of them, in three commits:forcexyzreassigned the capturedzinside itsapplyclosure, and calledGI.is3don the whole input, which throws aMethodErrorfor a vector of geometries even though the docstring says vectors work. It now checks each point, which is also what the docstring describes. A new test covers the vector case._build_a_listandlast_a_idxin theDoubleNaturalTreetraversal are nowRefs, and the conditionally reassignedmultipoly_ain the multipolygondifference/unionmethods islet-bound, so those now infer a concrete return type.reconstruct(GeometryOpsCore): the iteration state is threaded through aRef. This needs a GeometryOpsCore version bump, since GeometryOps pins=0.1.12.The 9th, in
ThinnedDoubleNaturalTree, is left alone: it's a typed box with no measurable cost, and that method gives wrong results anyway (#528).Julia 1.13, median, same results (hashes of the output coordinates) before and after:
forcexyz, 10k point polygonreconstruct, 25 polygons x 201 pointsintersectionwithDoubleNaturalTree, 4000 x 4000 verticesintersection, default (NestedLoop), 400 x 400differenceof two 3 x 200 multipolygonsunionof two 3 x 200 multipolygonsThe full test suite gives the same results as main, plus the two new
forcexyztests. CI will hit the staleoverlay_ng.jl:303test that already fails on main (fixed in #520).🤖 Generated with Claude Code