D-WXS-2a half A: the KILL fires — row-major vs Morton closed, no code change - #949
Conversation
The bar was committed before the run (d83b4d3), deliberately, because this deliverable exists to settle a deviation I introduced. Measured, over 20 pre-registered boxes (4 tile-aligned, 8 non-tile-aligned, 4 seam-crossing, 4 pole-adjacent) and a deterministic stride-677 neighbour sample of 1534 cells: arm rc median (non-tile-aligned) neighbour-locality median SHIPPED 140.00 32.00 MORTON 212.50 16.00 CONTROL-BAD 3100.00 15862.00 primary FAIL. The bar required MORTON to beat SHIPPED on BOTH metrics. It won one and lost one: 2x better on neighbour locality, ~1.5x worse on range count. control PASS. CONTROL-BAD is far worse on both, so the metrics measure locality rather than nothing. Without this the primary result would be unreadable. stay-silent twin PASS. Every tile-aligned box is exactly one range under BOTH arms. Section 1.2's load-bearing claim -- a 16-degree box is a HEEL-prefix scan -- holds, and shows no difference where the plan claims none. Pre-registered consequence applied unchanged: D-WXS-2a closes, no code change, the migration is not opened. A CORRECTION to my own section 1.3b wording, which is not a softening of the verdict. I phrased the KILL as "the deviation is harmless for this workload". The data shows that was imprecise: MORTON wins, 2x, on exactly the metric half B would care about -- neighbour locality is what a zeta stencil spends, range count is what a box scan spends. The honest statement is "no unambiguous win in key space", not "harmless". The verdict is untouched -- the bar said both, it got one, one is a FAIL, and re-reading a split as a win is the failure this arc has an explicit rule against. What changes is only the summary sentence that claimed more than the measurement. The prior this leaves for half B is recorded as a prior, not a result. The instrument was checked before the result was read. Selftest green, including the case the brief demanded be worked out rather than assumed: a full longitude row under SHIPPED is 23 ranges (one per longitude tile), not 1 -- because SHIPPED's second-most-significant byte is lon_tile, which sorts before the row-distinguishing lat_hip byte. Bijectivity of all three arms over all 1,038,240 cells is asserted, which is what licenses the exact rank computation and therefore the no-false-positives range count. SHIPPED's neighbour median is arithmetic-checkable and checks out: the order is lat_hip-major / lon_hip-minor within a tile, so lon+/-1 is 1 apart and lat+/-1 is 64 apart, median of [1,1,64,64] = 32, exactly the measured value. The probe's SHIPPED arm calls key.rs::encode_key THROUGH rather than re-deriving the shift/mask arithmetic, so it cannot drift from the shipped codec; MORTON and CONTROL-BAD are pure permutations of the same four bytes. 35/35, clippy -D warnings clean, fmt clean. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CcpLeEC3XK8Eye53GKBVvi
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f1b660fe-9922-46cd-98ea-0a10d51f8f07) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc7951ae1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn interleave_nibbles(lat: u8, lon: u8) -> [u8; 2] { | ||
| let lat_hi = lat >> 4; | ||
| let lat_lo = lat & 0x0F; | ||
| let lon_hi = lon >> 4; | ||
| let lon_lo = lon & 0x0F; | ||
| [(lat_hi << 4) | lon_hi, (lat_lo << 4) | lon_lo] |
There was a problem hiding this comment.
This implementation concatenates the axes' whole nibbles as lat_hi, lon_hi, lat_lo, lon_lo; it does not produce the repository's canonical Morton ordering, which alternates individual axis bits so every output nibble represents a 2-bit × 2-bit tile (FacetTier::morton in crates/lance-graph-contract/src/facet.rs and morton2 used by crates/perturbation-sim/src/cascade_key.rs). Consequently the committed metrics, claimed 2× locality prior, and closure of D-WXS-2a compare SHIPPED against a different layout than the OGAR-canon MORTON arm the experiment was intended to test; the arm must use the canonical bit interleave and the results be regenerated.
Useful? React with 👍 / 👎.
Follow-up to #948 (merged). Branch restarted from
main.Settles the undeclared deviation recorded in #948's plan §1.3a — the shipped key assigns one whole byte per axis where OGAR's cascade doctrine specifies the axis bytes nibble-interleaved (Morton).
The bar was committed before the run (
d83b4d3e), deliberately: this deliverable exists to settle a deviation I introduced, which makes writing the bar afterwards exactly the failure the rule guards.Measured
20 pre-registered boxes (4 tile-aligned, 8 non-tile-aligned, 4 seam-crossing, 4 pole-adjacent); deterministic stride-677 neighbour sample, n = 1534.
The three verdicts
[1,1,1,1]each). §1.2's load-bearing claim — a 16° box is a HEEL-prefix scan — holds, and shows no difference where the plan claims none.Pre-registered consequence, applied unchanged:
D-WXS-2acloses. No code change. The migration is not opened.⚠ A correction to my own §1.3b wording — not a softening of the verdict
§1.3b phrased the KILL as "the deviation is harmless for this workload". The data shows that was imprecise, and it is corrected in §1.3c rather than left to read as more than was measured:
MORTON wins, 2×, on exactly the metric half B would care about. Neighbour locality is what a ζ stencil spends; range count is what a box scan spends. The honest statement is "no unambiguous win in key space", not "harmless".
The verdict is untouched — the bar said both, it got one, and one is a FAIL. Re-reading a split as a win is the failure this arc has an explicit rule against ("re-normalising a metric until it stops showing the confound is how a confound becomes a finding"). What changes is only the summary sentence that claimed more than the measurement. The prior it leaves for half B is recorded as a prior, not a result — and a 2× locality win still would not by itself justify a migration, because the same measurement shows it costs ~1.5× on range count.
The instrument was checked before the result was read
Selftest green, including the case the brief demanded be worked out rather than assumed: a full longitude row under SHIPPED is 23 ranges (one per longitude tile), not 1 — because SHIPPED's second-most-significant byte is
lon_tile, which sorts before the row-distinguishinglat_hipbyte. Bijectivity of all three arms over all 1,038,240 cells is asserted, which is what licenses the exact rank computation and therefore the no-false-positives range count.SHIPPED's neighbour median is arithmetic-checkable and checks out: within a tile the order is
lat_hip-major /lon_hip-minor, solon ± 1is 1 apart andlat ± 1is 64 apart — median of[1, 1, 64, 64]= 32, exactly the measured value.The probe's SHIPPED arm calls
key.rs::encode_keythrough rather than re-deriving the shift/mask arithmetic, so it cannot drift from the shipped codec; MORTON and CONTROL-BAD are pure permutations of the same four bytes.35/35 · clippy
-D warningsclean · fmt clean.Still open, unchanged
D-WXS-0— the classid mint, OGAR-side and operator-gated. The bake must refuse to write until it resolves.D-WXS-7— the re-specifiedD-WXA-5, the actual gate. Never run.🤖 Generated with Claude Code
https://claude.ai/code/session_01CcpLeEC3XK8Eye53GKBVvi
Generated by Claude Code