Direct classification for degenerate descent cells (issue #107)#113
Direct classification for degenerate descent cells (issue #107)#113espg wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
=======================================
Coverage 94.33% 94.33%
=======================================
Files 9 9
Lines 1307 1307
=======================================
Hits 1233 1233
Misses 74 74
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
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of phase 1, including independent verification of the blocking claim.
Verdict on the blocking claim: CONFIRMED
The antisymmetry defect in ring_winding_at is real, and the PR's inversion of the #107 diagnosis is correct. Three independent lines of evidence:
- Derivation from the code (
src_rust/src/sphere.rs,ring_winding_at): for each edge term,pa = normalize(a − (a·x)x). Substituting−x:a − (a·(−x))(−x) = a − (a·x)x— the projections are bit-identical forxand−x; onlysgn = cross(pa,pb)·xflips. Sow(−x) = −w(x)exactly, term by term. An exactly-antisymmetric function cannot be2π·k(x); it is the azimuthal winding about thexaxis, which countsk(x) − k(−x). - The pinned test fails as claimed:
cargo test -- --ignored→test_point_in_ring_hemisphere_plus_antipodal_interiorpanics at(-5,0)("reads outside"), and the acceptance test fails with the parity-oracle divergence. The main suite is green (199 passed, 2 ignored — matches the PR body). - Independent oracle (standalone crate, plane-intersection crossing counts from locally-established anchors; shares nothing with the winding sum or the SoS predicates):
- Band ring (lat −10, CCW, interior = north): from a south-pole anchor (nearest edge 79.96° away, exterior side), probes (5,33), (−5,33), (0,121), (5,263) each cross the ring exactly once → truly INSIDE;
point_in_ring_robustreturns outside for all of them. (15,33) inside / (−20,33) outside — the regimes where the backend is right — also match. - #107 ring: two independent anchors, each classified purely locally ((30, 45.5): 0.433° right of edge 0 → exterior; (0, 350): 1.46° right of edge 4 → exterior) and mutually consistent (2 crossings between them). Both put centre(3,1) = (66.4435, 45) INSIDE (1 crossing, stable under lon 44.9/45.1 nudges) — so the issue #107 text had it inverted, exactly as this PR says. Its antipode (−66.44, 225 ± 0.1) is also inside — the lens condition,
w = k(x) − k(−x) = 0. (45,50) is outside (the exterior wedge) and (30,20) is inside (confirming the nudged control's cover was itself corrupted).
- Band ring (lat −10, CCW, interior = north): from a south-pole anchor (nearest edge 79.96° away, exterior side), probes (5,33), (−5,33), (0,121), (5,263) each cross the ring exactly once → truly INSIDE;
I agree with the consequence: phase 2's acceptance gate (debug_assert against parity_filled_robust) is unsatisfiable on this ring for any descent, since the oracle is wrong at every uniform cell in the lens. The a/b/c decision in "Questions for review" genuinely blocks phases 2–4; that decision is @-free here per convention and stays with the repo owner.
Verdict on the diff
No blocking bug found in the added code. chain_fill_from is a faithful extraction of base_fills' donor loop (argument-for-argument identical arcs_cross_sos invocation and parity fold; verified against the pre-diff code), the gate is conservative-safe (a false-positive gating only costs a direct classification), and the runtime path is unchanged by construction (both new items are dead_code-gated). cargo fmt clean; the clippy warnings on the branch all pre-date the PR (coverage/tests.rs 314/329/519). Inline comments:
- Should-fix (doc):
DirectClassifier::classify's "off every edge plane, with a trustworthy winding verdict" conflates two properties this PR itself proves distinct — donor fills inherit the lens defect; the reproducer ring's donors just happen to be lens-free (verified: pairs (1,11), (3,9), (4,6), (5,7) all straddle). - Should-fix (phase-2 question): donor acceptance margin equals
ORIENT_EPS, so a barely-off-plane donor yields a reference chord in the same near-coincident regime the gate rejects. - Nits: one comment inaccuracy about the existing band test's probe latitudes; borrowed
center_ids for non-centre probe points in the gate test.
Process notes: coverage.rs is now 1317 lines, already past the ~1000-line threshold of §4 before this PR — worth raising the split question before phase 2 adds the wiring. And since the PR now waits on an owner decision, it should carry the waiting label per §5.
Generated by Claude Code
| /// unambiguous reference (a base centre off every edge plane, with a | ||
| /// trustworthy winding verdict) and run one [`arcs_cross_sos`] pass over all |
There was a problem hiding this comment.
🤖 from Claude (review)
Should-fix (doc): "off every edge plane" does not imply "trustworthy winding verdict" — the docstring conflates the two, and this PR's own finding is the counterexample. seed_fill only checks plane proximity; per the antipodal-lens defect you pinned, an off-plane donor whose antipode is also interior gets a wrong parity_filled_robust fill, and classify then inverts every cell chained from it. Your PR body already documents a concrete instance (the nudged control: base 0 goes off-plane, takes the winding verdict, which is wrong there). I verified independently that for this reproducer ring all eight eligible donors are lens-free — pairs (1,11), (3,9), (4,6), (5,7) each straddle the boundary — which is the only reason test_direct_classifier_reproducer_centres can pass; that's a property of this ring, not of the mechanism. Suggest stating the dependency explicitly here (e.g. "donor fills come from the winding backend and are only sound where the donor's antipode is exterior — see the #107 blocking finding"), so phase 2 doesn't wire classify in the belief that the donor reference is already sound. The None => parity_filled_robust fallback inherits the same caveat.
Generated by Claude Code
| let donors = self.donors.get_or_init(|| { | ||
| let units: Vec<Vec3> = self.edges.iter().map(|e| normalize(&e.n_ab)).collect(); | ||
| (0..12u64) | ||
| .filter_map(|b| { | ||
| let c = cell_center_vec(0, b); | ||
| seed_fill(&c, &units, self.rings).map(|f| (c, center_id(0, b), f)) |
There was a problem hiding this comment.
🤖 from Claude (review)
Should-fix (question for phase 2): donor acceptance uses the same ORIENT_EPS the overlap gate fires on, so the reference chord itself can be #107-degenerate. A donor only has to clear each edge plane by ORIENT_EPS (1e-12). A donor at, say, 1.1e-12 off a long edge's plane, chained to a gated cell centre on that plane, gives a reference chord running within ~1e-12 of the edge's great circle over its whole span — exactly the near-coincident regime fill_leg_parity exists to reject, just below its trigger. Two cheap belt-and-braces options: (a) require donors to clear every plane by a margin ≫ ORIENT_EPS (the nearest-donor scan can just skip marginal ones — there are 12 candidates), or (b) run the gate's both-endpoints test on the chosen chord and fall through to the next-nearest donor when it fires. Fine to defer to phase 2, but worth deciding before classify is wired.
Minor, same block: classify with x bit-equal to a donor centre (zero-length chord, ir == ix into arcs_cross_sos) is unreachable in the intended use (gated cells are on-plane, donors off-plane), but nothing enforces it — a one-line debug_assert! or a documented precondition would be cheap insurance.
Generated by Claude Code
| // of −10 (hemisphere+). Every point with lat ∈ (−10, +10) has its | ||
| // antipode interior as well; the winding sum cancels to ~0 there and the | ||
| // backend calls a truly interior point outside. (The existing band test | ||
| // above only probes latitudes ≥ 13 or ≤ −11, and its cross-check oracle |
There was a problem hiding this comment.
🤖 from Claude (review)
Nit (comment accuracy): "the existing band test above only probes latitudes ≥ 13 or ≤ −11" isn't literally true — test_robust_pip_hemisphere_plus_band's full-sphere parity loop probes lat 0 and −9 (both in the lens). Those pass for the reason your second clause gives: they're only compared against winding_inside, a verbatim copy of the same antisymmetric sum, so backend and oracle are wrong together. The absolute (truth-anchored) assertions are indeed confined to lat 13 / −11 and up. Suggest rewording to something like "the existing band test's truth-anchored assertions only sit at latitudes ≥ 13 or ≤ −11; its in-lens probes (lat 0, −9) are checked only against another short-way winding sum, wrong in exactly the same region."
Generated by Claude Code
| latlon_to_unit_vec(0.0, 100.0), | ||
| latlon_to_unit_vec(0.0, 120.0), | ||
| ); | ||
| let (ip, iq) = (center_id(2, 5), center_id(2, 6)); |
There was a problem hiding this comment.
🤖 from Claude (review)
Nit: center_id(2, 5) / center_id(2, 6) are borrowed as SoS ids for probe points at (0, 100) and (0, 120), which are not those cells' centres. Functionally harmless here (the predicate only needs distinct, stable ids within the call), but center_id's doc sells these ids as load-bearing identities of specific centres, so the borrow reads as if the probes were real depth-2 centres. Two arbitrary literals in a clearly non-colliding range (or a one-line comment saying the ids are arbitrary) would avoid the false implication.
Generated by Claude Code
|
🤖 from Claude Phases 2–4 are blocked on a decision: the plan approved on issue #107 uses the winding PIP as the acceptance oracle (un-ignore the pinned test; parity-oracle fuzz), but phase 1 uncovered — and adversarial verification confirmed — that the oracle itself is broken for exactly the hemisphere+ family under test. The defect: Three independent confirmations:
Consequence: the debug parity oracle (which compares the descent against this PIP) can never pass on hemisphere+ inputs regardless of descent fixes, so the agreed acceptance gate for phases 2–4 is unreachable as specified. The approved fix direction ((a)-gate + (b)-direct-classify) is unaffected. Options (pick one):
One housekeeping flag while you're here: Applying Generated by Claude Code |
Closes #107
Implements the approved direction (approval comment): (b) direct classification for degenerate subtrees, with (a)'s collinearity detection as the trigger.
What phase 1 adds
fill_leg_parityinsrc_rust/src/coverage.rs: the fill-leg crossing walk now reportsNonewhen the leg overlaps a relevant edge's great circle (both probe endpoints withinORIENT_EPSof its plane — the Descent parity desynchronizes on hemisphere+ rings with a long edge collinear to the probe lattice #107 configuration). The two determinants it gates on (d_ab_p,d_ab_q) are the onesedge_crosses_probecomputes anyway, so the clean path is unchanged (edge_crosses_probewas split into a thin shell overedge_crosses_probe_dto share them).DirectClassifier::classify: one symbolicarcs_cross_sospass over all edges from the nearest unambiguous base-centre reference. The crossing-count mechanics were extracted frombase_fills' donor chain intochain_fill_from, whichbase_fillsnow calls — seeds and cells use one identical mechanism, per the recommendation. Donor discovery is lazy (OnceLock), so clean geometry pays nothing.Both new items carry a
#[cfg_attr(not(test), allow(dead_code))]marker that phase 2 (the descent wiring) removes.Blocking finding: the winding backend is wrong wherever a point and its antipode are both interior
While pinning phase 1's unit tests on the reproducer centres, the expected values did not match the issue's diagnosis, and the discrepancy traces to
ring_winding_at(src_rust/src/sphere.rs), the foundation ofpoint_in_ring_robust/parity_filled_robust— i.e. the seed PIP and the debug parity oracle itself.The defect (3-line proof). For an edge
(a, b)and test pointx, the summed term projectsa,bonto the plane ⊥x. That projection is identical for−x(a − (a·x)x = a − (a·(−x))(−x)); only the sign factordot(cross(pa, pb), x)flips. So the whole sum is antisymmetric:w(−x) = −w(x). A function with that symmetry cannot equal the true winding indicatork(x) ∈ {0, 1}— it computes2π·[k(x) − k(−x)]. Consequences:xinterior, antipode exterior →+2π(correct); both exterior →0(correct); both interior →0(wrong: truly-interior point reads outside).Canonical reproducer (pinned as the
#[ignore]dtest_point_in_ring_hemisphere_plus_antipodal_interiorinsrc_rust/src/sphere/tests.rs): the lat −10 band from #22's own tests, CCW interior = everything north of −10. Every point with lat ∈ (−10, 10) has an interior antipode; the backend returns0for all of them:The existing
test_robust_pip_hemisphere_plus_bandmisses this because its probe latitudes skip (−11, 13), and its cross-check oracle is a second short-way winding sum — wrong in exactly the same region.This inverts part of the #107 diagnosis.
centre(3,1)at (66.44, 45) is truly inside (walk(45,50)[exterior wedge] →(60,50): crosses edge 1 exactly once;(60,50)→centre(3,1): crosses nothing). The issue read it as outside because the winding said 0 — but−centre(3,1) = (−66.44, 225)is also interior, so the winding is untrustworthy precisely there. The corrected facts are pinned intest_direct_classifier_reproducer_centres— noteclassifyalready returns the true verdict at that centre, because its donor's antipode is exterior (the regime where the winding verdict is sound).Both measurement references in the issue were affected:
parity oracle diverged at uniform cell (15, 2)etc.), and its release cover excludes truly-interior(30, 20)while the exact ring includes it. The 8221-cell symmetric difference compared two differently-wrong covers.centre(3,1)'s cell missing though interior;(45, 50)'s cell covered though exterior), so a real descent-side desync also exists — the issue's core observation stands, but the acceptance oracle cannot adjudicate the fix.Why this blocks phases 2–4 as approved: phase 2's acceptance gate is un-ignoring
test_descent_hemisphere_ring_collinear_edge_oracle, i.e. the debugdebug_assert_eq!(node.fill, parity_filled_robust(...))passing on this ring. Even a perfect descent fails that assert at every uniform cell in the antipodal lens (correct fill = inside, oracle = outside). Phase 3's fuzz harness "under the parity oracle" would likewise fail on essentially any hemisphere+ ring, collinear edges or not. No descent-side change can get there; the reference itself needs repair first.Phases
fill_leg_parity) +DirectClassifieronbase_fills' donor machinery (chain_fill_fromreuse); unit tests on the reproducer centres; winding-backend defect pinned as an#[ignore]d testnode_children; un-ignore the acceptance test (blocked: see "Questions for review")How it was tested
cargo test: 199 passed, 0 failed, 2 ignored (the acceptance test, still ignored with an updated reason; the new winding-defect pin).cargo fmt/cargo clippy --all-targets: clean (remaining warnings pre-date this PR).maturin develop --release && pytest -v: 670 passed, 11 skipped.flake8 mortie --select=E9,F63,F7,F82: clean.base_fills' donor chain is a pure extraction; the gate and classifier are not yet wired), so the release suite is unchanged by construction.Questions for review
|w| = 2πis definitive (+2π⇒ anchor inside,−2π⇒ anchor outside, both because that requires the antipode on the other side) — then classify anyxasanchor_fill XOR crossing_parity(anchor → x)viaarcs_cross_sos. That is exactly the "edge-crossing PIP from a fixed reference" thatsphere.rs's layer-2 comment defers, and the same shape asbase_fills' donor chain / this PR'sclassify_direct. Options: (a) extend this PR to repairpoint_in_ring_robustfirst (new phase between 1 and 2), then complete phases 2–4 against the repaired oracle; (b) keep this PR to the approved scope, file the winding defect as its own issue, and mark this PR blocked by it; (c) something else. I lean (a) — phases 2–4 are unreachable otherwise — but it widens scope beyond the approved recommendation, so flagging rather than proceeding.w ≡ 0everywhere (no anchor). I believe no simple spherical polygon with finitely many great-circle edges can have an antipodally self-symmetric interior other than exact-hemisphere cases, but the repair should document its policy there (fall back to current behavior?).DirectClassifierpicks the nearest unambiguous base-centre donor (keeps the reference chord short). If all 12 base centres are ambiguous (boundary planes through every centre), it falls back to the raw winding verdict, mirroringbase_fills. Happy to escalate to depth-1 centres instead if you prefer belt-and-braces here.Generated by Claude Code