Repository navigation
fix: cap the expansion hop count instead of hanging near the poles - #33
Conversation
|
This change is part of the following stack:
Change managed by git-spice. |
There was a problem hiding this comment.
Pull request overview
Prevents geohash expansion from effectively hanging near the poles by capping the computed expansion hop count and surfacing a clear error when an expansion would require an extreme/degenerate number of rings. This fits into the library’s “geography expansion” utilities and improves safety for both Rust and Python callers.
Changes:
- Introduces
MAX_EXPANSION_HOPSand adds validation in hop sizing to refuse runaway polar expansions (and other degenerate sizing inputs) with actionable errors. - Splits hop sizing into
n_hops_for_core -> Result<usize, String>plus a thinPyResultwrapper to keep the policy testable fromcargo test. - Adds Rust unit tests and Python tests covering the runaway polar case and ensuring large-but-reasonable expansions are still allowed; updates docstrings to reflect
min(height, width)sizing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/lib.rs |
Adds hop-count capping + degenerate-input rejection in expansion sizing; adds Rust tests and updates expansion docstrings. |
tests/test_geohash_ops.py |
Adds Python regression tests for polar runaway expansions and a sanity test for large allowed expansions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if !min_cell_m.is_finite() || min_cell_m <= 0.0 { | ||
| return Err(format!( | ||
| "cannot size an expansion against geohash {sample_hash:?}: its cell has no \ | ||
| usable extent (width {cell_width_m} m, height {cell_height_m} m)" | ||
| )); | ||
| } | ||
|
|
||
| let hops = (expansion_m / min_cell_m).ceil(); | ||
| if hops > MAX_EXPANSION_HOPS as f64 { | ||
| return Err(format!( | ||
| "expanding by {expansion_m} m from geohash {sample_hash:?} needs {hops:.0} hops, \ | ||
| over the limit of {MAX_EXPANSION_HOPS}. The cell is only {min_cell_m:.3} m across \ | ||
| at its narrowest — use a coarser precision, or a smaller expansion_m." | ||
| )); | ||
| } | ||
| Ok(hops as usize) |
There was a problem hiding this comment.
Leaving this one as is. The message already names the geohash and both extents (width {cell_width_m} m, height {cell_height_m} m), which is the diagnostic content — min_cell_m is just the smaller of the two, and the hop count on this branch is inf or NaN by construction, so printing it adds a number that can never be anything else. The branch exists precisely because the hop count is meaningless here.
33d98fc to
ece4a59
Compare
65ce366 to
2257266
Compare
2257266 to
dc47d92
Compare
ece4a59 to
2256bc4
Compare
dc47d92 to
237ffcd
Compare
2256bc4 to
129071d
Compare
n_hops_for sizes an expansion against the smaller of a cell's height and width. Cell width shrinks with cos(latitude), so the hop count climbs steeply toward the poles. At precision 9, expanding 1 km needs 419 hops at latitude 60, 1,206 at latitude 80, and around 1.5e8 in the top row. Each hop adds a ring, so a polar input built a frontier of billions of cells and never returned. If the narrowest extent ever reached zero the division gave +inf, `as usize` saturated to usize::MAX, and the loop could not terminate at all. Refuse both cases with a message naming the offending hash, the hop count and the cell's narrowest extent, so the caller can see whether to coarsen the precision or shrink expansion_m. The cap is 10,000 hops: far past any real expansion, far short of what a degenerate input produces. A 50 km expansion at precision 6 still goes through. Splits the sizing logic into n_hops_for_core, which returns Result<_, String>, from the thin PyResult wrapper. Constructing a PyErr pulls in Python symbols the Rust test binary does not link, so the policy was otherwise unreachable from cargo test. Also corrects two docstrings that said the hop count comes from a cell's height; it has used min(height, width) since the function was written. Squashed with: - fix: size a group's expansion on its narrowest cell, not its first hash Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
237ffcd to
824c376
Compare
129071d to
0e25cfe
Compare
Problem
n_hops_forsizes an expansion against the smaller of a cell's height and width. Cell width shrinks withcos(latitude), so the hop count climbs steeply toward the poles:Each hop adds a ring, so a polar input built a frontier of billions of cells and never returned. If the narrowest extent ever reached zero, the division gave
+inf,as usizesaturated tousize::MAX, and the loop could not terminate at all.Fix
Refuse both cases with a message naming the offending hash, the hop count and the cell's narrowest extent, so the caller can see whether to coarsen the precision or shrink
expansion_m. The cap is 10,000 hops — far past any real expansion, far short of what a degenerate input produces. A 50 km expansion at precision 6 still goes through.Splits the sizing logic into
n_hops_for_corereturningResult<_, String>from the thinPyResultwrapper: constructing aPyErrpulls in Python symbols the Rust test binary does not link, so the policy was otherwise unreachable fromcargo test.Also
Corrects two docstrings that said the hop count comes from a cell's height; it has used
min(height, width)since the function was written.