perf: rhealpixdggs 0.8.5 hierarchical polyfill, with a per-geometry min_res fix - #217
Merged
Merged
Conversation
rhealpixdggs 0.8.5 rewrote polyfill as a hierarchical descent from a min_res parameter (default 0) down to the target resolution, plus a new polyfill_array (same semantics, returns a numpy array instead of a set). It's a large win for big/complex polygons - benchmarked 7x-118x faster than the old algorithm on rhealpixdggs's own NZ-scale reference polygon, reaching resolutions (3.4M cells in 6.6s) the old algorithm couldn't practically reach. But defaulting to min_res=0 for every geometry is a ~900x regression on typical small real-world polygons: tests/data/chathams-pannz-2014.gpkg (159 real features, mostly small parcels) went from 0.38s (old algorithm) to 345s. Root cause: the new algorithm pays a roughly fixed vectorised cost per hierarchy level descended, wasted almost entirely when only a handful of cells are ever in play at each of many "wasted" coarse levels before reaching a small polygon's actual location. Fixed with RHPVectorIndexer._fill_min_res: choose min_res per geometry from its own bounding-box extent, reusing the existing DGGS_CELL_AREA_M2_BY_RES table (rHP cells are equal-area squares, so width = sqrt(area), confirmed identical to WGS84_003.cell_width() to the last decimal). With this, the same 159-feature fixture at resolution 9 runs in 1.05s - faster than the old algorithm's 1.68s, output bit-for-bit identical. One more hazard: a geometry that touches a pole or spans all 360deg of longitude gets the whole polar square as its candidate region regardless of how small it actually is - confirmed a tiny pole-touching bbox produces a ~42 million cell starting frontier at min_res=8. A bbox-diagonal heuristic alone would pick a deep min_res for such a geometry and hang, so these two cases are explicitly clamped to min_res=0. min_res is not a correctness parameter - rhealpixdggs's own test suite asserts output is min_res-invariant - so no fudge-factor margin is used, only the pole/antimeridian clamp above. Also: - Switched _polyfill_polygon to polyfill_array, which also fixes a latent nondeterminism: the old set()-based path's row order depended on Python's string-hash randomisation across runs; polyfill_array's output is documented-sorted. - Fixed the compact_cells import, which moved from rhealpixdggs.rhp_wrappers to rhealpixdggs.conversion between 0.8.3 and 0.8.5 (rhp_wrappers no longer re-exports it). - Bumped the pyproject.toml constraint to ^0.8.5 (the new code calls min_res=/polyfill_array, absent in 0.8.3) and relocked. New tests/classes/rhp_fill.py: unit tests for _fill_min_res including the pole/antimeridian clamp, correctness/equivalence tests against min_res=0 and min_res=resolution (the pre-0.8.5 algorithm's own semantics), and performance-regression guards (a mocked min_res-is-passed check plus generous wall-clock budgets using the real fixture). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Bumps rHP's
rhealpixdggsdependency to^0.8.5, which rewrotepolyfillas a hierarchical descent from amin_resparameter (default 0). Adopting it naively is dramatic in both directions:min_res=0—tests/data/chathams-pannz-2014.gpkg(159 real features, mostly small parcels) went from 0.38s to 345s for identical output.The fix
RHPVectorIndexer._fill_min_reschoosesmin_resper geometry from its own bounding-box extent, reusing the existingDGGS_CELL_AREA_M2_BY_REStable (rHP cells are equal-area squares, so width = √area — confirmed identical toWGS84_003.cell_width()to the last decimal). With this, the same 159-feature fixture at resolution 9 runs in 1.05s — faster than the old algorithm's 1.68s, output bit-for-bit identical.min_resis not a correctness parameter (rhealpixdggs's own tests assert output ismin_res-invariant), so no fudge-factor margin is used. The one exception: a geometry touching a pole or spanning all 360° of longitude gets the whole polar square as its candidate region regardless of size — confirmed a tiny pole-touching bbox produces a ~42 million cell starting frontier atmin_res=8— so those two cases are explicitly clamped tomin_res=0.Also in this PR
_polyfill_polygontopolyfill_array, which also fixes a latent nondeterminism: the oldset()-based path's row order depended on Python's string-hash randomisation across runs;polyfill_array's output is documented-sorted.compact_cellsimport, which moved fromrhealpixdggs.rhp_wrapperstorhealpixdggs.conversionbetween 0.8.3 and 0.8.5 (rhp_wrappersno longer re-exports it) — a genuine break unrelated to polyfill, found while testing.New tests (
tests/classes/rhp_fill.py)_fill_min_res, including the pole/antimeridian clampmin_res=0andmin_res=resolution(the pre-0.8.5 algorithm's own semantics)Testing
This sandbox can't install packages, so verification used
PYTHONPATH-shadowing the pure-Python 0.8.5 source (no compiled extensions, confirmed loads cleanly ahead of the locally-installed 0.8.3):black,ruff check .,mypy vector2dggs/— all cleanpsycopg2-missing failures)poetry.lockrelocked successfully; the 0.8.5 wheel hash matches raster2dggs's own lock for the same version independentlyPlease run the full suite once more against the real installed 0.8.5 wheel (
poetry install -E all --with dev) before merging, as final confirmation against the published artifact rather than the source-shadowed checkout.🤖 Generated with Claude Code