Group A small-fix bundle from issue #108#111
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
- Coverage 94.33% 94.33% -0.01%
==========================================
Files 9 9
Lines 1307 1323 +16
==========================================
+ Hits 1233 1248 +15
- Misses 74 75 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Solid bundle overall: fix (1) matches the issue and is properly tested; fix (2)'s inventory claim verified empirically (clippy on main: exactly 37 useless_conversion; on this branch: 0 suppressed lints, precisely the 6 claimed pre-existing warnings remain); and fix (3)'s deviation from the issue's literal |Σ ring areas| trigger is well-reasoned — the fan sum does wrap mod 4π exactly where the literal check would need it, and keying on exact covered area is the right call for that failure mode. Findings, ranked: (should-fix) the guard does not make the winding sign trustworthy as the new comments claim — a ~1.3 sr equatorial band (verified end-to-end on this branch) wraps both ring areas to −5.63, fires the global flip, and dies in a cryptic stitcher panic instead of the new actionable ValueError; a cheap cross-check of Σ fan areas against the exact covered area would close the hole (details inline on dissolve). (should-fix, trivial) a truncated leftover comment dangles above classify_and_split (dissolve.rs 420–421). (nit) cover_area's disjoint-cells precondition is undocumented (duplicates double-count toward the threshold); (nit) moc=True, normalize=False on polygonal geometry remains a silent no-op while the linear branch now fails loud — worth an explicit espg call on uniformity.
Generated by Claude Code
| pub fn dissolve(morton: &[u64], step: u32) -> Result<ClassifiedRings, String> { | ||
| let area = cover_area(morton); | ||
| if area > std::f64::consts::TAU - HEMISPHERE_MARGIN { | ||
| return Err(format!( | ||
| "dissolved cover spans {area:.6} sr — within 2% of a hemisphere \ | ||
| (2π sr) or beyond — so its exterior/hole winding is ambiguous; \ | ||
| split the cover into sub-hemisphere parts or pass dissolve=False \ | ||
| for per-cell polygons" | ||
| )); | ||
| } |
There was a problem hiding this comment.
🤖 from Claude (review)
The exact-area key is sound for what it checks, but it does not deliver what the new comments claim. The mod-4π wrap is a function of the area each ring encloses, not of the covered area — so a small cover whose rings individually enclose more than a hemisphere still wraps, sails through this guard, and still breaks classification.
Verified end-to-end on this branch (throwaway clone, scratch test in this module): an equatorial band of 320 order-4 cells covering |lat| ≲ 3.7°:
band cells=320 area=1.3089969389957445 (guard threshold=6.157521601035994)
boundary rings=2 ring_areas=[-5.628269441174673, -5.628269441174666] sum=-11.25653888234934
thread ... panicked at src_rust/src/dissolve.rs:372:5:
unbalanced antimeridian segments but no pole enclosed
Each boundary ring encloses a ~2π+0.65 sr cap, so the fan returns the wrapped complement (−5.628 instead of +6.94), Σ = A − 4π < 0, the global flip fires on a correctly-wound cover, and the stitcher dies on its assert — surfaced to Python as the cryptic dissolve panicked: unbalanced antimeridian segments but no pole enclosed instead of this item's curated, actionable ValueError. (This family is pre-existing on main — classify_and_split is unchanged — so it's not a regression; but the PR's whole point per issue #108 was "fail loud instead of silently inverting", and this polar-adjacent shape still fails cryptic.)
Cheap hardening that would close it: dissolve already has the exact area in hand — pass it into classify_and_split and, after computing the fan areas, reject when (total.abs() - area).abs() is large (any wrapped ring is off by ~4π; a tolerance of, say, π has no false-positive surface). That converts every wrap — near-2π covers and hemisphere-enclosing-ring covers — into the same actionable error, and makes the "sign is trustworthy" comments below actually true. At minimum, the comments at lines 432–433 (and the geometry.py mirror) should be softened back to an assumption, because as written they overclaim.
Generated by Claude Code
| // Winding-guard margin (issue #108): spherical signed area is defined mod 4π, | ||
| // so a cover whose |Σ ring areas| lands near 2π reads the same as its |
There was a problem hiding this comment.
🤖 from Claude (review)
Leftover edit artifact: this comment is truncated mid-sentence ("…reads the same as its") and duplicates the HEMISPHERE_MARGIN block at the top of the file — it also dangles over classify_and_split, which defines no "winding-guard margin". Delete these two lines.
Generated by Claude Code
| /// Exact covered area (steradians) of a morton cover: Σ π/(3·4^depth). | ||
| fn cover_area(morton: &[u64]) -> f64 { | ||
| morton | ||
| .iter() | ||
| .map(|&w| std::f64::consts::PI / (3.0 * 4f64.powi(mort2nested(w).1 as i32))) | ||
| .sum() | ||
| } |
There was a problem hiding this comment.
🤖 from Claude (review)
Nit: "exact" holds only for disjoint, non-duplicated words — duplicates and parent/child overlaps double-count, so a legitimate sub-threshold cover containing duplicates can spuriously raise. That precondition is real in practice: raw morton_coverage output can carry duplicates (the test helper _polar_cap that feeds the new over test needs its np.unique). Worth one line in this doc comment (mirrored in _reject_hemisphere_cover) stating the disjoint-cells precondition, or a dedup in both engines — whichever, keep the two mirrors in lockstep.
Generated by Claude Code
| # (Spherical signed area is defined mod 4π; the hemisphere guard above | ||
| # keeps the cover well under 2π, so the sign is trustworthy.) |
There was a problem hiding this comment.
🤖 from Claude (review)
Same overclaim as the Rust twin (see the main comment on dissolve in src_rust/src/dissolve.rs): the guard bounds the covered area, but the wrap is driven by the area each ring encloses — an equatorial band of ~1.3 sr (well under 2π) still wraps both rings to −5.63, flips the winding here, and dies in _next_segment's RuntimeError instead of the new curated ValueError. "keeps the cover well under 2π, so the sign is trustworthy" should either become true (cross-check sum(areas) against the exact covered area the guard just computed) or stay phrased as the assumption it was before this change.
Generated by Claude Code
| if not normalize: | ||
| raise ValueError("normalize applies only to polygonal geometry") |
There was a problem hiding this comment.
🤖 from Claude (review)
Nit / question for espg: the fix matches issue #108 Group A item 1 exactly and the test pins both LineString and MultiLineString. But the same "silent no-op" rationale from #97 item 7 now applies one branch up: moc=True, normalize=False on polygonal geometry is still silently ignored (morton_coverage_moc has no orientation auto-correct at all), while the linear branch fails loud. The updated docstring enshrines the asymmetry ("Ignored when moc=True"). Defensible as-is since the issue scoped this to the linear branch — flagging in case fail-loud was meant to be the uniform policy.
Generated by Claude Code
2 participants
Refs #108 (Group A only — Group B items remain open on that issue, so no
Closes).One bundled
small-fixPR per §5, one commit per item, plus one follow-up commit addressing the adversarial self-review (review):from_geometry(..., normalize=False)linestring guard — the linear branch offrom_geometrynow raisesValueError("normalize applies only to polygonal geometry")instead of silently ignoringnormalize=False, symmetric with the existingmoc/tolerance/max_cellsrejection two lines above (mortie/geometry.py). Docstring updated; testtest_ingest_linear_rejects_normalize_falsecovers LineString and MultiLineString viafrom_wkt. (Consolidated follow-ups from the June/July sweeps (#73, #88, #97) #108 Group A item 1, from Follow up items for the week of July 4, 2026 #97 item 7)useless_conversion—#[allow(clippy::useless_conversion)]at the top ofsrc_rust/src/lib.rswith a one-line comment that the pyo3/numpy?conversions are load-bearing.cargo clippy --all-targetswarning count: 43 → 6 (exactly the 37useless_conversioninstances suppressed). The 6 remaining warnings (3cloned_ref_to_slice_refs, 2useless_vec, 1needless_range_loop— all pre-existing, mostly in test code) stay visible, confirming nothing else is masked. (Consolidated follow-ups from the June/July sweeps (#73, #88, #97) #108 Group A item 2, from Follow up items for the week of June 23, 2026 #73 item 8)dissolve(Rustsrc_rust/src/dissolve.rs, surfaced as a PythonValueErrorfromrust_dissolve, mirrored in the Python oracle_dissolved_rings_pyvia_reject_hemisphere_cover) now fails loud on covers spanning near or over a hemisphere instead of silently swapping exteriors and holes. Tests: Rusthemisphere_cover_fails_loud/sub_hemisphere_cover_still_dissolves, Pythontest_dissolve_hemisphere_cover_fails_loud(exact-hemisphere and over-hemisphere polar cap, both engines, plus thedissolve=Falsefallback). (Consolidated follow-ups from the June/July sweeps (#73, #88, #97) #108 Group A item 3, from Follow up items for the week of July 4, 2026 #97 item 5)4f43894addresses the self-review findings:classify_and_splitnow takes the exact covered area and rejects with a curatedValueErrorwhen||Σ ring areas| − area| > π(an honest fan matches the exact area within chord discretization ≲0.1 sr; any wrap is off by ~4π, so π has no false-positive surface). Mirrored in the Python oracle (same failure mode there,RuntimeErrorin_next_segment); the "sign is trustworthy" comments are now backed by the cross-check. Tests: Rustequatorial_band_fails_loud_not_cryptic, Pythontest_dissolve_hemisphere_enclosing_ring_fails_loud(both engines).classify_and_split(finding): removed.cover_areadisjoint-cells precondition (finding): documented in both mirrors (cover_areadoc comment and_reject_hemisphere_coverdocstring) rather than deduping — duplicate words already break the edge-cancellation dissolve itself, so disjoint input is the function's existing contract.Note on the guard key for (3)
The issue's standing recommendation was to raise when
|Σ ring areas| ≈ 2π. While implementing it I found the literal check does not fire on the flagship ambiguous input: for a cover of exactly half the sphere (24 order-1 cells, base cells 0–5), the anchor-fan signed-area formula wraps mod 4π per-ring and returnsΣ = −1.85, not±2π— and because ring chaining starts from aHashMapiteration, the wrapped value's sign is not even deterministic at that scale. Probing polar caps:Σtracks the true area faithfully below ~2π (e.g. 6.02 for a 6.02 sr cap) and only breaks at/past the hemisphere.So the guard is keyed on the exact covered area instead — cells are equal-area, so
Σ π/(3·4^depth)over the input words is exact — raising whenarea > 2π − marginwithmargin = 2% of 2π(HEMISPHERE_MARGIN). This honors the item's intent (fail loud when the winding sign becomes untrustworthy), fires on the exact-hemisphere case the literal check misses, and additionally catches all over-hemisphere covers (which today silently emit the complement). The review follow-up adds the complementary per-ring wrap detection (Σ cross-checked against the exact area), so hemisphere-enclosing rings on small covers now also fail loud instead of panicking. The error messages tell the user to split the cover or passdissolve=False.How tested
cargo test: 200 passed, 0 failed (3 new dissolve tests);cargo fmtclean;cargo clippy --all-targets: 6 pre-existing warnings, 0useless_conversion.maturin develop --release && pytest -v: 673 passed, 11 skipped (3 new Python tests).flake8 mortie --select=E9,F63,F7,F82: clean.Questions for review
2π·0.02 ≈ 0.126 sr) is a judgment call — it keeps the guard clear of the fan-formula breakdown while still admitting a 96%-of-hemisphere polar cap (which dissolves correctly today). The wrap cross-check tolerance (π) is separately justified above. Happy to tighten/loosen either.|Σ ring areas|for the reasons above — flagging the deviation from the issue's wording explicitly in case the literal form was wanted anyway.ValueErrorwhere they previously surfaced a cryptic stitcher panic — behavior changes beyond "near 2π", but the old outputs were wrong or unactionable. The full winding-free classifier from Follow up items for the week of July 4, 2026 #97 item 5 remains a possible follow-up if polar-scale dissolves become a real workload.moc=True, normalize=Falseon polygonal geometry is still a silent no-op (morton_coverage_mochas no orientation auto-correct at all), while the linear branch now fails loud — the docstring documents the asymmetry. Issue Consolidated follow-ups from the June/July sweeps (#73, #88, #97) #108 scoped item 1 to the linear branch, so this PR leaves the polygonal branch unchanged; say the word if fail-loud should be uniform and it can ride this PR or a follow-up.