SpatialIndex follow-ups: query_nearest, index persistence, and a constructor fix - #124
Open
espg wants to merge 12 commits into
Open
SpatialIndex follow-ups: query_nearest, index persistence, and a constructor fix#124espg wants to merge 12 commits into
espg wants to merge 12 commits into
Conversation
espg
force-pushed
the
feature/spatial-index-followups
branch
from
August 9, 2026 04:37
09052d4 to
e9758ed
Compare
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.
Follow-up to the #118 review thread (the
query_nearestsplit from that discussion) plus the serialization piece from #72.Motivation
Same pipeline as #118: mapping NASA CMR granule footprints to spatial workers in zagg. The index over the full 555,867-granule catalog is for the full ICESat-2 mission to date, so identical for every query against that mission, and the whole build cost is per-process waste after the first invoke.
555,867 polygons (17.68M vertices) against 2,721 query polygons, single-threaded, M1 Max. The mortie column is the HEALPix/MOC path the same pipeline uses, for scale:
¹ elementwise
spherely.intersects; killed at 20 min, never completed.² 8.35 s building geographies + 0.05 s constructor (14.97 before this branch's fix) + 10.07 s
build(tmp_memory_budget=4 GiB)(198.34 at the default — see #118) + ~1 sencode().³ parquet defaults; ~350 MB with zstd and tuned row groups.
So ~19.5 s once and 0.65 s per process after, against ~18.5 s in every process that wants to query.
query_nearestis the distance-query half requested in #72; the encode work is the "serialize the index / memory-map and query" idea from the same thread. Baselines in espg#2.Right now, we default to mortie and pay around a minute; that could get dropped by ~2x from optimizations in that library. Building an index in mortie has high memory usage, and the index improvement is marginal (mainly a result of the input geometries, which are thin and cross many cells). #118 plus the constructor fix below gets ~25 s — a 2.6× speedup over the current pipeline — even building and discarding the index every time. But having this merged drops the query time by more than 10x after the first index build is persisted.
The other benefit is separation of concerns; larger machine builds the index and pays the memory overhead, follow on users that hit the index do it memory efficient at small runtimes.
This diff includes #118's commits; the new work is the last
45 commits (query_nearest,encode,from_encoded, constructor, bugfix).1.
query_nearest— shapelySTRtree.query_nearestparityS2ClosestEdgeQueryover the existing index. Distances in units of aradiuskeyword defaulting toEARTH_RADIUS_METERS, matchingspherely.distance; interiors count on both sides, also matching it.Two behaviours worth flagging:
all_matches=Falsereturns the lowest tied index rather than an arbitrary one — deterministic, and a valid instance of shapely's documented "arbitrary".max_distanceis genuinely inclusive: the converted bound is widened a few ULP, because the meters→S1ChordAngleround trip otherwise disagrees with the reported distance by ~2 ULP, so re-querying with a distance the call just returned could come back empty. A single fixture passes by luck ~72% of the time; pinned by a randomized boundary test.Correctness is pinned against the independent
spherely.distancepath on randomized trees, including antimeridian-crossing and pole-adjacent geometry and >2-way exact ties.2. Index persistence — build once, ship the index, open ~instantly
encode()(defaultinclude_geographies=True) writes a full-fidelity blob: candidate and predicate queries,query_nearest,exclusiveand.geometriesall work on the decoded index, through the same code paths as a built index — no second implementation of predicate semantics to drift. Geographies are reconstructed lazily per entry on first touch and cached; opening a 5k-entry blob is ~0.1 ms, and candidate-only queries reconstruct zero geographies.include_geographies=Falsewrites a structure-only blob: candidate queries andquery_nearestwork; predicate refinement,exclusiveand.geometriesraise, naming the full-fidelity option as the remedy.ShapeFactory, so shape data is stored once. Full-fidelity blobs run ≈1.1× (polygon-heavy) to ≈2× (small point-only) the size of structure-only ones.CompactEncodeTaggedShapes,MutableS2ShapeIndex::Encode,Geography::EncodeTagged).EncodeTagged/DecodeTaggedare experimental upstream, so the docs say plainly this is a build-coupled cache/ship format tied to the bundled s2geometry/s2geography versions — not archival.Init, per-block kind verification. Corrupt or truncated input raisesValueError, fuzzed over every truncation prefix and a set of crafted mutations. A corrupt block first touched from inside a distance query surfaces cleanly and leaves the index usable.3. Constructor: bypassing
s2geography::GeographyIndex::AddThe last commit adds shapes to the wrapped
MutableShapeIndex()directly, keeping the shape id → tree index mapping in spherely.Addgrows that mapping withvalues_.reserve(values_.size() + num_shapes)thenresize(new_shape_id + 1)per shape. libc++ reserves exactly and theresizeleavescapacity == size, so every subsequentAddreallocates and copies the whole vector — quadratic in geography count. On a 555,867-polygon catalog that is 14.974 s of pure vector growth; doing it here takes 0.048 s (312×), with a byte-identicalencode()blob — same index, same fill rule, including multi-shape and empty geographies.The cost: the wrapped
GeographyIndex's ownvalues_is now permanently empty, so itsvalue()andIteratormust not be used. spherely uses neither (it already walks the shape index itself) and there is a comment saying so, but it is a sharp edge. Three options here — first one implemented since it fixes it now (question 4 below):GeographyIndexhere for a plainMutableS2ShapeIndex. Nothing else in spherely uses the wrapper, so the hazard disappears — but so does the clean revert.values_geometrically, take it as a version floor. Two-line change on their side; happy to open that PR.Testing
293 passing (181 at the #118 head). The one touch this branch makes to #118-reviewed code — the candidate cell walk now uses a generic
S2ShapeIndex::Iteratorso built and decoded indexes share one query path — is pinned by a differential test, byte-identical to the #118 branch across 1,644 comparisons spanning all geometry kinds, predicates, and antimeridian/pole cases. Built-vs-decoded parity is tested exhaustively (candidates, all predicates, nearest indices and distances, multi-shape collections, empties), plus laziness pins via an internal decode counter, concurrent-query stress, and a GIL-release test onencode(). mypy, black and clang-format clean; warning-free under-Wconversion -Wsign-conversion.Questions for review
from_encodedacceptsbytes/bytearray/memoryviewand copies once. A zero-copy/mmap path — the "memory map it and query it" idea from Querying geographies (spatial index) #72 — is a natural follow-up if there's appetite;EncodedS2ShapeIndexis built for it.Geography.__setstate__against crafted state tuples. Left out since pickle is pre-existing surface — separate PR if wanted.