Skip to content

Uniform symbolic crossing predicate: fix base-cell-boundary mis-fill#106

Merged
espg merged 6 commits into
mainfrom
claude/103-robust-boundary-crossing
Jul 10, 2026
Merged

Uniform symbolic crossing predicate: fix base-cell-boundary mis-fill#106
espg merged 6 commits into
mainfrom
claude/103-robust-boundary-crossing

Conversation

@espg

@espg espg commented Jul 10, 2026

Copy link
Copy Markdown
Owner

Closes #103. Refs #78, #87, #44 (the prior per-predicate hardenings this generalizes), #90 (re-scoped on the back of this PR), #107 (the follow-up limitation this PR's oracle discovered).

What this does

Implements the plan agreed on the issue thread (approach (1), sign-off + closed-set semantics): one uniform symbolic crossing predicate for every sidedness decision in the coverage descent. All the issue's reproducers now come back in-band (see "How it was tested").

Phases

  • Phase 1 — the predicate + differential tests. arcs_cross_sos in src_rust/src/sphere.rs: arcs cross iff the four orientations [a c b], [c b d], [b d a], [d a c] share a sign (the S2 SimpleCrossing identity), each through the SoS-hardened orient_sos. No constructed intersection point. Five test groups incl. the exact morton_coverage / from_wkt mis-fill polygons whose edges lie on base-cell boundaries (lon ≡ 0 mod 45°, lat = 0) #103 parity-breaking configuration (pinned) and vertex-graze-counts-once.
  • Phase 2 — wiring + the deeper fixes the parity oracle forced. Call-site swap (old robust_crossing/on_minor_arc pipeline deleted); closed-set touch semantics with a segment gate (edge_hits_cell_edge); the parity oracle debug_assert (uniform-cell fill must equal the independent winding PIP); globally unique UNIQ-coded SoS ids for cell centres (positional ids made the fill chain path-dependent at boundary-coincident centres); seed hardening (seed_fill geometric ambiguity detection + base_fills donor chaining); exact determinant signs under orient_sos (Shewchuk error-free expansions, std-only, no new dependency, filter-guarded) — near-coplanar noise signs were jointly inconsistent across triples, the residual morton_coverage / from_wkt mis-fill polygons whose edges lie on base-cell boundaries (lon ≡ 0 mod 45°, lat = 0) #103 mechanism off the bit-exact grid.
  • Phase 3 — regression matrix (mortie/tests/test_coverage_boundary.py, 87 tests): all six degenerate meridians incl. 315 × three latitude bands × both sides × both hemispheres; equator edges incl. through (0,0); the non-monotone width family; flagship reproducers at original orders; winding invariance. Asserts superset / no-escape (bulge-aware) / flat == densified MOC.
  • Phase 4 — folded self-review findings. Major: base_fills' donor chain ran ~122° chords through the bare two-straddle fast path (antipodal phantom crossings) — now unconditionally symbolic, reviewer's reproducer pinned. Medium: the closed-set span gate's cos-space epsilon → angular (sin ρ-scaled). Plus docstring precondition, rustdoc link, stale docs.
  • Phase 5 — perf recovery for the CodSpeed regression. The −15..35% on clean-geometry coverage benches traced to edge_hits_cell_edge computing all four determinants up front where arcs_cross_n exited after two on the common corners-same-side case (the straddle-test hot path), plus per-seed re-normalization of edge normals in seed_fill. The two-dot early exit is restored (same shape as the retired arcs_cross_n), unit normals are computed once per descent, and center_id is hoisted out of the corner loops. One documented closed-set nuance from the fast exit: a pure vertex-point touch on a cell edge with the polygon strictly outside is guaranteed only for the vertex's leaf-owning cell; edge-collinear touches — the systematic on-grid family the contract exists for — are always degenerate-path and fully honored. Awaiting CodSpeed re-measure on this head; the exact-arithmetic layer itself is filter-guarded and was never the cost.

How it was tested

  • cargo test — 197 passed (parity oracle live in the test profile), 1 deliberately #[ignore]d (Descent parity desynchronizes on hemisphere+ rings with a long edge collinear to the probe lattice #107). cargo clippy no new warnings; cargo fmt --check clean.
  • maturin develop --release + pytest635 passed (87 new matrix tests), 11 skipped; flake8 clean.
  • Issue reproducers, before → after at the issue's orders: belt lon[0,5] o9 1687-cells-spanning-the-globe → 1887 in-band (control 1886); cap lon[44,45] o6 829-to-the-pole → 13 in-band; 315° mirror-symmetric (13); southern mirror 13; ring sector o9 775-reaching-−89.9 → 87, max dip −87.26 vs the polygon's own chord dip of −87.53.

Questions for review

  1. Known limitation → filed as Descent parity desynchronizes on hemisphere+ rings with a long edge collinear to the probe lattice #107 with the full diagnosis per your go-ahead (#[ignore]d acceptance test in-tree).
  2. Module sizes (sphere/tests.rs, coverage.rs past ~1000 lines) — deferred to the 1.x freeze sweep (v1.0: full codebase review for the 1.x freeze #68) per your call.
  3. Evaluate relaxing node_straddles containment now that on_minor_arc is SoS-hardened #90re-scoped per your ask (comment): unblocked by this PR, baseline moves here, relaxation target excludes closed-set touch refinements, parity oracle as guard rail.
  4. New (phase 5): is the vertex-point-touch nuance above acceptable as the closed-set contract's fine print, or do you want the neighbor cell guaranteed too (costs the two extra dots on the hot path — roughly the difference CodSpeed flagged)?

@espg espg added the implement label Jul 10, 2026
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.33%. Comparing base (4fb4cd2) to head (50029f5).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
+ Coverage   93.73%   94.33%   +0.59%     
==========================================
  Files           9        9              
  Lines        1230     1307      +77     
==========================================
+ Hits         1153     1233      +80     
+ Misses         77       74       -3     
Flag Coverage Δ
unittests 94.33% <ø> (+0.59%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 2 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b92649...50029f5. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed-hq

codspeed-hq Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will degrade performance by 17.67%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 14 regressed benchmarks
✅ 53 untouched benchmarks
⏩ 1 skipped benchmark1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
coverage_triangle_polar[8] 851.8 µs 1,246.9 µs -31.69%
coverage_triangle_polar[6] 347.5 µs 455.6 µs -23.74%
coverage_triangle[4] 108.5 µs 139.1 µs -21.98%
coverage_square_polar[4] 193.4 µs 238.7 µs -18.99%
test_coverage_triangle_order8 1.2 ms 1.4 ms -17.95%
coverage_triangle_polar[4] 202.4 µs 242.8 µs -16.64%
order6[500] 2.5 ms 2.9 ms -16.26%
coverage_circle500[6] 2.5 ms 2.9 ms -16.11%
test_coverage_circle500_order6 2.7 ms 3.2 ms -15.56%
coverage_square_polar[8] 1.1 ms 1.3 ms -14.88%
coverage_square_polar[6] 466.8 µs 545.1 µs -14.37%
coverage_circle500[8] 3.3 ms 3.8 ms -12.9%
order6[100] 711.5 µs 811.6 µs -12.33%
test_coverage_triangle_order6 457.5 µs 517.5 µs -11.59%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing claude/103-robust-boundary-crossing (50029f5) with main (faa2410)

Open in CodSpeed

Footnotes

  1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports.

@espg espg left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

Adversarial review of phase 1 (arcs_cross_sos + tests) at head 163efcf, verified with an out-of-tree fuzz harness built from this commit's sphere.rs against an independent angular-betweenness reference.

Verified correct:

  • The 4-sign identity (S2 SimpleCrossing, i.e. det[a c b], [c b d], [b d a], [d a c] all one sign): 2,000,000 random quadruples, arc lengths 1e-4 … ~π rad, 1/3 with CD deliberately placed near the AB-midpoint antipode — 0 disagreements with the geometric reference and 0 with robust_crossing on non-degenerate inputs. The antipodal-separation class that fools the bare straddle is rejected by the identity, as the new test claims.
  • Early-return structure is equivalent to "all four signs equal" because orient_sos is total (never 0) — signs are ±1, so the equality chain is exact.
  • Vertex graze counts once: 200,000 bit-exact meridian (y == 0) vertex passages with randomized distinct ids — opposite-side incident edges always total exactly 1 crossing; same-side (graze) always even.
  • Collinear-edge parity (the issue-#103 shape): 200,000 boxes with the west edge exactly on the probe circle, random latitudes and random id assignments — 4-edge crossing count always even. The pinned [true, false, true, false] is id-assignment-specific, but the adjacent relabeling-parity test covers the invariant that matters, so the pin works as a regression fixture.
  • Reorder invariance exact under arc swap/reversal, including bit-identical shared endpoints with distinct ids (625k checks) — see the one inline caveat about duplicated ids.
  • The differential grid test exercises all 5,040 quadruples (584 crossing-true), so checked > 1000 holds with wide margin; its 1e-9 degeneracy filter never fires on this point set (harmless, purely protective).
  • Conventions: commit style matches the repo (phase 1 of issue #103), no new deps, cargo fmt --check clean, clippy introduces no new warnings, full cargo test at this commit: 196 passed.

Findings — both Low, inline: (1) document the pairwise-distinct-id precondition on arcs_cross_sos (silent consistency loss demonstrated when violated); (2) sphere/tests.rs crossed the ~1000-line CLAUDE.md §4 guideline and phases 2–4 will add more — needs a split-or-exempt call.

No blocking issues found in phase 1.


Generated by Claude Code

Comment thread src_rust/src/sphere.rs Outdated
Comment on lines +449 to +450
/// instead of being hand-maintained. Total and reorder-invariant; `ia, ib,
/// ic, id` are the SoS identities of the four endpoints.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

Low — state the pairwise-distinct-id precondition here. "Total and reorder-invariant" holds only when ia, ib, ic, id are pairwise distinct. With a duplicated id, orient_sos's canonical sort can no longer order the two points and the guarantee fails silently in the near-coplanar noise zone: in a 500k fuzz where c reused a's id (points distinct, one near the AB great circle), arcs_cross_sos(a,b,c,d,…) != arcs_cross_sos(c,d,a,b,…) in 184,761 cases (~37%). [PointId]'s doc does say identities must be "distinct and consistently ordered", but this predicate is the surface phase 2 wires call sites to, so a one-line "requires pairwise-distinct ids" in this docstring would make the contract greppable at the point of use.

The converse is solid and worth noting for the same reason: bit-identical points with distinct ids stay exactly reorder-invariant (verified over 125k shared-endpoint quadruples, 0 failures) — that's the property the incident-edge vertex-graze argument rests on.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Folded in phase 4: the docstring now states the pairwise-distinct-id precondition at the point of use (and notes that bit-identical points with distinct ids remain exactly invariant — the property the vertex-graze argument rests on).


Generated by Claude Code

]
}

// ── arcs_cross_sos: the uniform symbolic crossing predicate (issue #103) ──

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

Low (convention) — sphere/tests.rs is now 1092 lines, past CLAUDE.md §4's "~1000 lines without prior discussion" guideline, and phases 2–4 plan to add more suites here (parity oracle, seed-PIP, regression matrix). Flagging per the convention rather than as a defect — a quick call from espg would settle it: (a) split the crossing-predicate suites into their own file (e.g. sphere/tests_crossing.rs) before phase 2 lands, or (b) treat test modules as exempt from the guideline. Either answer is fine; it just shouldn't grow another 500 lines silently.


Generated by Claude Code

@espg espg left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

Fresh-context adversarial review of the phase-2 commit (163efcf..4412cfd, "phase 2 of issue #103"). I attacked the exact arithmetic, the SoS id scheme, the seed chaining, the closed-set gate, and the parity oracle, verifying every claim by running code (scratch worktree at 4412cfd; nothing in the repo was modified). Three findings, ranked; the inline comments carry the reproducers.

Findings

  1. Major — base_fills donor chain counts phantom antipodal crossings (inline on base_fills). edge_crosses_probe's fast path is the bare two-straddle, sound in the descent only because probes are short and edges relevance-culled; the donor chain feeds it ~122° base-centre chords over all edges. A polygon that separates the antipodes of the chain endpoints gets an inverted seed → whole base subtree mis-filled in release; in debug your own parity oracle catches it (which is exactly the oracle earning its keep). Verified end-to-end with a 5-vertex reproducer: debug panics at parity oracle diverged at uniform cell (15, 2); release differs from the 1e-6°-nudged control by 8221 of ~22k cells at order 6. Fix is cheap: use arcs_cross_sos unconditionally in the chain.
  2. Medium — edge_hits_cell_edge segment gate: ORIENT_EPS (a triple-product magnitude) reused as cos-of-angle slack; angular window floors at ~1.4e-6 rad for short segments (≈10³ cell-widths at order 29). Gate-level verified; end-to-end exposure limited to short on-meridian edges at high order (inline).
  3. Minor — stale doc: new rustdoc broken-intra-doc-link warning for the deleted robust_crossing at sphere.rs:484 (inline).

What held up under attack (all verified, not just read):

  • two_sum is the branch-free Knuth TwoSum, exact. two_product via f64::mul_add is exact (Rust guarantees single rounding; residual representable barring denormal underflow, unreachable for unit vectors).
  • exact_sum_sign is GROW-EXPANSION (Shewchuk Thm. 10) with zero elimination — expansion kept nonoverlapping and ascending, sign = last component. 20,000 adversarial trials (exact-cancelling pairs, mixed exponents −60..60, up to 24 terms) against an exact rational oracle: 0 mismatches. Same for the 24-term orient_exact_sign fallback on near-coplanar triples.
  • The mag * 1e-15 filter: for dot(a, cross(b, c)) evaluation order the worst-case rounding error is ~6u·mag ≈ 6.7e-16·mag (u = 2⁻⁵³); empirical max over 200k trials 1.85e-16·mag. So 1e-15 is a safe overestimate — margin ~1.5×, so "comfortably above" in the comment is a mild overstatement, but the accept side is sound and the reject side is exact.
  • antipodal_base: verified against cell_center_vec(0, ..) — all six pairs dot to < −1 + 1e-12, all 60 other pairs are far from antipodal.
  • center_id: exhaustively unique over depths 0–6, top bit set, and the depth-29 maximum stays clear of CORNER_ID_A/B. The stable-id design (rings perturb first, one global perturbation) is the right call and the doc explains why.
  • Parity oracle: no side effects in the assert expression; fully compiled out in release; I found no spurious-fire mechanism (a uniform cell's centre is ≥ its inradius from the boundary — any closer and the closed-set straddle test refines the cell first — so the winding verdict there is trustworthy; hemisphere+/multipart use the same parity_filled_robust backend on both sides). It caught finding (1) instantly.
  • seed_fill's 1e-12 plane gate: winding noise at the trust boundary is ~1e-16 rad-equivalent, four orders inside the gate — no false-trust window. 12 seeds × E normalizes is negligible.
  • cargo fmt --check clean; cargo clippy exits 0 (all warnings pre-existing in lib.rs/old tests); 196 tests pass; commit style matches; no new dependencies (the ~90-line expansion kernel honors the no-new-deps agreement).

Conventions: coverage.rs is now 1136 lines, past the ~1000 soft limit — and coverage/tests.rs's own header says the tests were split out precisely to stay under it. The PR body's "Questions for review" flags sphere/tests.rs but not this; same treatment question applies (the seed_fill/base_fills/antipodal_base block or the edge predicates are natural split seams). Also: the new kernels (exact_sum_sign, orient_exact_sign, minor_exact_sign, edge_hits_cell_edge, seed_fill/base_fills, center_id) have no direct Rust unit tests — the phase-3 matrix exercises them only through small-box geometries, which is exactly why finding (1) slipped past 196 green tests. A regression test derived from the inline reproducer would pin both the fix and the oracle.

Findings (2) and (3) are non-blocking for the next phase; (1) is a correctness bug in shipped release behavior and worth a phase commit before the PR leaves draft.


Generated by Claude Code

Comment thread src_rust/src/coverage.rs Outdated
Comment on lines +674 to +692
let all: Vec<usize> = (0..edges.len()).collect();
for b in 0..12u64 {
if known[b as usize] {
continue;
}
// A non-antipodal known donor always exists: each base has exactly one
// antipodal partner, and at least one other seed is known.
let d = (0..12u64)
.find(|&d| known[d as usize] && !antipodal_base(b, d))
.expect("a non-antipodal known donor base centre");
let parity = arc_crossing_parity(
&centers[d as usize],
&centers[b as usize],
center_id(0, d),
center_id(0, b),
&all,
edges,
);
fill[b as usize] = fill[d as usize] ^ parity;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[1] Major (verified end-to-end): the donor chain counts phantom antipodal crossings — an ambiguous seed can be chained to the inverted fill, flipping its whole base subtree in release builds.

edge_crosses_probe's fast path is the bare two-straddle

(d_pq_a > 0.0) != (d_pq_b > 0.0) && (d_ab_p > 0.0) != (d_ab_q > 0.0)

which is satisfied not only by a real crossing but also when the edge sits on the far intersection of the two great circles (edge contains y, probe contains -y). The descent never sees this because its probes are short (centre→child/corner) and the edge list is relevance-culled, so no surviving edge is antipodal to a probe. base_fills breaks both shields at once: it feeds base-centre chords up to ~122° long over all edges (let all = (0..edges.len()).collect()).

Per-edge, the phantom fires exactly when the edge crosses the antipodal image of the probe arc; summed over a closed ring, the chained parity is inverted iff the polygon separates the antipodes of the two chain endpoints — for the c1 → c0 chain, iff exactly one of base centres 10 and 11 is inside the polygon.

Reproducer (verified against 4412cfd; ring vertices (lat, lon)):

(10,45) (50,45) (-10,170) (-70,225) (-10,280)
  • the (10,45)→(50,45) meridian edge makes seed_fill(c0) return None → donor chain from centre 1;
  • arc_crossing_parity(c1, c0, …, &all, …) returns false, while the same count through arcs_cross_sos per edge is true (two edges of this ring cross the probe's antipodal arc region; one of them phantoms);
  • debug build: polygon_to_morton_coverage(&lats, &lons, 4, true) panics on this phase's own parity oracle — parity oracle diverged at uniform cell (15, 2) (and others) — the oracle works exactly as designed;
  • release build: silently wrong output. Same polygon with the meridian nudged to lon = 45.000001 (no ambiguity, no chain): symmetric difference of 8221 cells out of ~22k at order 6.

The phase-3 matrix can't catch this: its reproducers are small boxes, whose boundaries never reach the antipodal arc of a donor chord (M = 0, even, parity survives).

Suggested fix: in the donor chain, route every edge through arcs_cross_sos unconditionally instead of edge_crosses_probe — worst case 12 seeds × E edges, once per call, off the hot path, and arcs_cross_sos handles the antipodal configuration correctly (I verified its verdicts on these probes against dense arc sampling). Two related notes while in here: (a) known[b] = true after chaining makes a chained seed eligible as a donor for later seeds, so an inverted chain compounds; (b) seed_fill measures ambiguity against each edge's infinite great circle, so seeds get chained more often than strictly necessary — harmless once the chain itself is exact, but today it widens the exposure to this bug.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Folded in phase 4: the donor chain now routes every edge through arcs_cross_sos unconditionally (no edge_crosses_probe, no fast path), and your reproducer ring is pinned as test_base_fills_chain_no_antipodal_phantom — every winding-classifiable seed must agree with the chain. On your two notes: (a) donor eligibility of chained seeds is now benign — the chain is exact, so a chained seed carries the exact verdict and compounds nothing; (b) the infinite-great-circle ambiguity test stays deliberately wide since chaining is now always correct and the cost is 11 arcs at most.

One thing your reproducer surfaced beyond this finding: even with the chain fixed, the full descent on that ring still trips the parity oracle — the 40°-long (10,45)→(50,45) edge is collinear with the probe lattice, and the vertex-graze bookkeeping between the collinear edge and its transversal sibling desynchronizes on the hemisphere+ ring. That is the long-arc territory sphere.rs's own comments defer, it predates this PR (the oracle is what made it visible), and it's documented as an #[ignore]d test (test_descent_hemisphere_ring_collinear_edge_oracle) plus a question for review on the PR.


Generated by Claude Code

Comment thread src_rust/src/coverage.rs Outdated
Comment on lines +536 to +548
if (d1 == 0.0 && dot(&e.mid, c1) >= e.cos_rho - ORIENT_EPS)
|| (d2 == 0.0 && dot(&e.mid, c2) >= e.cos_rho - ORIENT_EPS)
{
return true;
}
if d3 == 0.0 || d4 == 0.0 {
let mid = normalize(&[c1[0] + c2[0], c1[1] + c2[1], c1[2] + c2[2]]);
let cos_rho = dot(&mid, c1);
if (d3 == 0.0 && dot(&mid, &e.a) >= cos_rho - ORIENT_EPS)
|| (d4 == 0.0 && dot(&mid, &e.b) >= cos_rho - ORIENT_EPS)
{
return true;
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[2] Medium (gate-level verified): the segment gate reuses ORIENT_EPS — a triple-product magnitude — as a cos-of-angle slack, and the angular window blows up as sin_rho → 0.

dot(mid, x) >= cos_rho - ORIENT_EPS admits points up to Δ beyond the segment endpoint, where Δ·sin_rho + Δ²/2 ≤ 1e-12, i.e. Δ ≈ 1e-12/sin_rho for long segments but a floor of Δ ≈ √(2e-12) ≈ 1.4e-6 rad for short ones. Measured against the committed formulas:

  • d3/d4 branch (polygon endpoint vs cell-edge cap): for an order-29-scale cell edge (~1.5e-9 rad), cos_rho rounds to 1.0 and sin_rho to 0.0, so the gate degrades to a pure absolute window — a polygon vertex bit-exactly on the meridian passes as "within the segment" from 1.4e-6 rad away, ≈ 10³ cell-widths.
  • d1/d2 branch: a cell corner ~0.8e-6 rad past the endpoint of a 1.7e-6-rad polygon edge still gates as a touch.

Two mitigating facts I confirmed: bit-exact d == 0.0 incidence needs lon ≡ 0 (mod 90°) (at lon 45, cos45°/sin45° differ by 1 ulp, d ≈ 1e-22 ≠ 0, and those correctly route to the arcs_cross_sos branch below); and edge_relevant's own 1e-12 cos-slack culls the distant cells again unless the polygon edge is also short (≲ 2e-6 rad ≈ 13 m). So the end-to-end exposure is: finely segmentized boundaries lying on an exact 0/90/180/270 meridian at order ≳ 21 — narrow, but squarely inside the on-grid family this PR is about, and it inflates the agreed closed-set contract ("touches exactly") into "touches, or is within ~1.4e-6 rad along the great circle past the endpoint". I verified the gate arithmetic only, not a full-coverage strip.

Suggestion: keep the slack in angle space — e.g. require dot(mid, x) >= cos_rho - K·sin_rho with K an angular epsilon (plus an exact tie at the endpoint itself), or decide segment membership with the two endpoint half-plane signs through the exact orient machinery this PR already adds, which needs no epsilon at all.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Folded in phase 4: the slack is now angular — dot ≥ cos_ρ − sin_ρ·ε − ε² (both branches, with the cell-edge branch computing its own sin_ρ), so the window stays ~1e-12 rad for short segments instead of flooring at √(2e-12). The endpoint-exact tie still gates (the −ε² term covers cos rounding at ρ→0). Your no-epsilon alternative (endpoint half-plane signs through the exact machinery) is the cleaner endgame; noted on the PR as the follow-up shape if the closed-set contract ever needs to be tolerance-free end-to-end.


Generated by Claude Code

Comment thread src_rust/src/sphere.rs

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[3] Minor: deleting robust_crossing left a broken intra-doc link. cargo doc --no-deps now emits a new warning (rustdoc::broken_intra_doc_links):

warning: unresolved link to `robust_crossing`
   --> src_rust/src/sphere.rs:484:55
484 | /// The companion SoS predicates [`orient_sos`] and [`robust_crossing`] are the

That paragraph on point_in_ring_robust also still describes the retired pipeline as a companion ("building blocks the descent's per-cell parity flips will use in a later phase") — worth rewording to arcs_cross_sos while fixing the link. Cosmetic and optional: the test_robust_crossing_* test names in sphere/tests.rs now exercise arcs_cross_sos; a rename would keep future greps honest.

Everything else I attacked in this file held up — details in the review summary: two_sum/two_product/exact_sum_sign verified against an exact rational oracle on 20k adversarial trials (0 mismatches), the 1e-15 filter bound verified analytically (~6.7e-16 worst case for this evaluation order) and empirically (max observed noise 1.85e-16·mag over 200k trials).


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Doc link and the stale "companion pipeline" paragraph fixed in phase 4 (now points at arcs_cross_sos). The test_robust_crossing_* names are left standing deliberately — they pin behavioral continuity across the swap; happy to rename in the 1.x freeze sweep if you'd rather keep greps clean.


Generated by Claude Code

@espg espg added the waiting label Jul 10, 2026
@espg

espg commented Jul 10, 2026

Copy link
Copy Markdown
Owner Author

🤖 from Claude

All four phases complete; local gates green (197 cargo tests with the parity oracle live, 635 pytest incl. the 87-test boundary matrix, clippy/fmt/flake8 clean); both self-review rounds' findings folded or answered on-thread. Marking waiting for your review — kept draft per the routine's drafts-only rule (same note as PR #105). Three items under "Questions for review": the deliberately-out-of-scope hemisphere+/collinear-edge limitation the oracle uncovered (recommend a follow-up issue — yours to open), the module-size call, and the #90 connection. Watch the CodSpeed report for the exact-predicate perf gate; the exact path is filter-guarded so clean geometry shouldn't move.


Generated by Claude Code

@espg

espg commented Jul 10, 2026

Copy link
Copy Markdown
Owner Author

Known limitation discovered by the oracle, deliberately out of scope (documented as the #[ignore]d test_descent_hemisphere_ring_collinear_edge_oracle): on a hemisphere+ ring whose 40°-long edge is collinear with the probe lattice (the phase-2 reviewer's adversarial ring (10,45),(50,45),(-10,170),(-70,225),(-10,280)), the descent's vertex-graze bookkeeping between the collinear edge and its transversal sibling can still desynchronize. This predates the PR — the old noise-sign machinery was strictly weaker on the same family; the new oracle is what made it visible — and it's the long-arc consistency work sphere.rs's own comments defer. Recommend a follow-up issue (opening one is yours to call); happy to write it up with the full diagnosis.

yes, write it up with full diagnosis as a new issue

Module sizes: sphere/tests.rs (~1250 lines) and coverage.rs (~1180 lines) are past the ~1000 guideline. Both grew from this fix's tests/kernels. Split now (mechanical) or defer to the 1.x freeze sweep (#68)? Your call either way.

we can defer on this

Re #90: with the disambiguation now exact end-to-end, the node_straddles over-refinement is the only remaining conservatism; the measurement #90 calls for can now be done meaningfully. Left out of scope here per the original agreement.

I'm confused-- what are you saying here? Is this something that's made possible due to the work in this PR? or is #90 no longer needed and be closed because of this PR? or... is it irrelevant and orthogonal? What's your specific recommendation?

@espg espg marked this pull request as ready for review July 10, 2026 06:12
@espg

espg commented Jul 10, 2026

Copy link
Copy Markdown
Owner Author

🤖 from Claude

What's your specific recommendation [on #90]?

Concretely: #90 stays open, and this PR is what makes it actionable — recommend keeping it, re-scoped one notch. Before this PR, the node_straddles over-refinement was load-bearing: it was the safety net that papered over the crossing predicate's residual float sensitivity (the old coverage.rs:442 caveat), so relaxing it risked exposing wrong fills. After this PR the predicate is exact end-to-end, so the net is no longer hiding correctness bugs — it's now pure conservatism, and #90's "measure the over-refinement, then decide whether to relax" program can be run safely, with the parity oracle as the guard rail the measurement never had. Two adjustments to #90's text when you next touch it: (a) the measurement baseline should be this branch, not pre-#87 main; (b) one nuance — the closed-set contract from #103 deliberately adds boundary-touch refinement, so the relaxation target is the non-touching straddle over-refinement only. Not closable, not orthogonal: unblocked.

Separately: CodSpeed's −19% on clean-geometry coverage benches looks real (25 benchmarks, consistent direction), which contradicts my "filter-guarded, clean geometry shouldn't move" claim — investigating now; prime suspect is edge_hits_cell_edge losing arcs_cross_n's two-dot early exit on the quad-test hot path. Fix lands as phase 5.


Generated by Claude Code

@espg espg merged commit 999730b into main Jul 10, 2026
20 of 21 checks passed
@espg

espg commented Jul 10, 2026

Copy link
Copy Markdown
Owner Author

🤖 from Claude

Final perf report (from the merge-commit benchmark run + the branch head's CodSpeed analysis): the merged state measures −17.67% overall — 14 regressed coverage benches, worst coverage_triangle_polar[8] at −31.7%, coverage_circle500[6] −16.1%. Phase 5 recovered only part of the original −19.4%.

Callgrind attribution found the two real causes — neither is the exact-arithmetic layer (filter-guarded; absent from every profile):

  1. base_fills classified all 12 base seeds per call (12 winding PIPs + a 12×E plane scan) where the old base_node paid only for cap-surviving bases (1–3 typical);
  2. edge_hits_cell_edge grew past the inline threshold, so the quad-test hot loop paid a real function call per test.

Both fixed in follow-up PR #109 (draft, implement): seed classification respects the cap cull, and the hot shell is #[inline(always)] with the closed-set logic outlined #[cold]. Local callgrind puts the triangle case at +1% vs pre-#106 and the 500-vertex circle at −5% (below baseline); CodSpeed on #109 will confirm. Semantics untouched — the full #103 boundary matrix passes unchanged.


Generated by Claude Code

@espg espg mentioned this pull request Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

morton_coverage / from_wkt mis-fill polygons whose edges lie on base-cell boundaries (lon ≡ 0 mod 45°, lat = 0)

2 participants