refresh README benchmarks + cross-order table (issue #65)#131
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Phase 1 fresh-context self-review (diff vs CLAUDE.md + issue #65 acceptance criteria). Findings inline. Summary:
- One actionable, diff-scoped item: document that the coverage
msis a single cold sample (the encode/decode columns are warm median-of-5). Will fold a one-line note into the doc during Phase 2/3. - Two confirmations (loop-variable closure is safe here; throughput denominator is correct).
No blockers for advancing to Phase 2. Acceptance-criteria check: generator is committed and self-writing between markers (mirrors bench_matrix.py), cell counts deterministic, timings disclaimed — all met for the cross-order table itself. README retire + bench_matrix.py regen land in Phase 2; reproducibility note in Phase 3.
Generated by Claude Code
| # coverage: mixed-order cover of the small fixed box (rep=1, no warmup — | ||
| # order 29 is seconds-scale). | ||
| t_cov, cov = timed( | ||
| lambda: mortie.morton_coverage_moc(BOX_LAT, BOX_LON, order=order), 1) |
There was a problem hiding this comment.
🤖 from Claude (review)
Methodology asymmetry: encode/decode are timed warm as median-of-5, but coverage is a single cold rep=1 sample with no warmup. The coverage ms is therefore noisier and not strictly comparable to the encode/decode columns. That's a deliberate trade (order-29 coverage is ~3 s, so median-of-5 would add ~15 s to the run), and timings are already disclaimed as machine/run-dependent — but the doc's "Notes on the numbers" should say the coverage timing is a single sample so nobody over-reads a 5% wobble between runs. Cell counts are unaffected (deterministic). Folding a one-line note into the Phase 2/3 doc edit.
Generated by Claude Code
| for order in ORDERS: | ||
| # encode: geo2mort(lat, lon, order) -> uint64 morton array | ||
| morton = mortie.geo2mort(lat, lon, order=order) # warmup + decode input | ||
| t_enc, _ = timed(lambda: mortie.geo2mort(lat, lon, order=order), 5) |
There was a problem hiding this comment.
🤖 from Claude (review)
The lambdas close over the loop variable order (and morton), which is a classic late-binding footgun — but here it's safe: timed() invokes the lambda synchronously within the same iteration before order advances, so each measurement uses the intended order. Noting it only so a future reader doesn't "fix" it into a bug; no change needed. Same pattern as bench_matrix.py's lambda: fn(la, lo, order).
Generated by Claude Code
| # decode: mort2geo(morton) -> (lat, lon) | ||
| mortie.mort2geo(morton) # warmup | ||
| t_dec, _ = timed(lambda: mortie.mort2geo(morton), 5) | ||
| dec_mps = N / t_dec / 1e6 |
There was a problem hiding this comment.
🤖 from Claude (review)
Throughput uses N (element count) for both encode and decode, which is correct: mort2geo returns a 2-tuple of N-length arrays, so N morton indices are decoded per call regardless of the tuple shape. M idx/s = N / median_seconds / 1e6 is the honest per-index rate. Good — just confirming the denominator isn't accidentally double-counting the lat+lon outputs.
Generated by Claude Code
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Phase 2 fresh-context self-review (README retire + docs/coverage_methods.md regen).
- Actionable inline: the "roughly flat across orders 4–29" wording contradicts the linked table (~2x drop). Fixing in a follow-up commit.
docs/coverage_methods.md: diff is timings-only — the four cell-count columns are byte-identical to the prior committed matrix, which is the right invariant (counts deterministic,msmachine-dependent). The static flood-fill prose above the table (2,989 ms / 45.8 s / "~40–60x") is unchanged and out of scope here.- README no longer quotes hand-pasted absolute numbers (only "tens of millions … per second"), so it won't go stale as the docs table is regenerated — matches @espg's "keep the README lean" decision.
Generated by Claude Code
| Python implementation to fall back on — so performance is reported as **absolute | ||
| throughput** rather than a speedup ratio. Encoding (`geo2mort`) and decoding | ||
| (`mort2geo`) run at **tens of millions of morton indices per second** on one | ||
| core, roughly flat across orders 4–29. |
There was a problem hiding this comment.
🤖 from Claude (review)
"roughly flat across orders 4–29" overstates it — the generated table this line points to shows encode 44.0 -> 22.6 M idx/s and decode 20.8 -> 11.6 M idx/s from order 4 to 29, i.e. about a 2x drop, not flat. A reader who clicks through will notice the mismatch. Softening to "within roughly 2x across orders 4–29" in a follow-up so the README claim matches the linked numbers.
Generated by Claude Code
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)
Phase 3 fresh-context self-review (reproducibility section in docs/benchmarks.md).
- Mirrors the
bench_matrix.pydocumentation pattern; both commands are the real one-liners that rewrite their target docs between markers — verified end-to-end in this run. - Also folded in the Phase 1 finding here: the notes now state encode/decode are warm median-of-five and the coverage
msis a single cold sample. - One non-blocking observation inline (build needs a virtualenv; deferred to
BUILDING.md).
All three phases complete. Remaining follow-up before handing off: the Phase 2 "roughly flat" wording fix (committing now).
Generated by Claude Code
| ```bash | ||
| python bench_cross_order.py # this page: cross-order encode/decode/coverage | ||
| python bench_matrix.py # docs/coverage_methods.md: coverage-method matrix | ||
| ``` |
There was a problem hiding this comment.
🤖 from Claude (review)
The reproducibility section mirrors how bench_matrix.py is documented in coverage_methods.md (self-writing between markers, run from repo root) — good, that was the acceptance criterion. One honesty gap worth a follow-up: maturin develop --release requires an active virtualenv (it aborts with "Couldn't find a virtualenv" otherwise), which bit me setting this up. BUILDING.md is linked and should cover it, so I'm leaving the one-liner as-is rather than duplicating build prerequisites here — flagging only so it's a conscious choice, not an oversight.
Generated by Claude Code
|
🤖 from Claude Status: all three phases complete and pushed; awaiting review. Marking Local gate results
Pre-existing failure flag (not introduced here, not fixed here per CLAUDE.md §4):
Self-reviews for all three phases are posted inline above (prefixed Generated by Claude Code |
|
🤖 from Claude Two follow-ups in 1. It wasn't a regression — the number was a cold-call artifact. I built the pre-#103 baseline (
The #106 exact-predicate swap (issue #103, 2026-07-10) did cost coverage briefly, but the next commit 2. The apparent 2× came from this bench's coverage column, now fixed. It timed
The ratio is inversely proportional to per-call work, which is worth knowing on its own: Gates: Separately: this doesn't touch the "should benchmarks auto-update on release" question — my read is timings should not be auto-written into docs from shared CI runners (run-to-run variance exceeds what we're measuring); deterministic cell counts can be, and CodSpeed ( Generated by Claude Code |
|
🤖 from Claude Follow-up in Because the first-call cost is process-global and one-time — only the very first parallel call is cold; every later matrix cell is already warm — a per-cell "cold" column across the matrix would be misleading. So each warm-up row is measured as a genuine first call in its own fresh subprocess, contrasting cover sizes:
That's the actual insight: the penalty is inversely proportional to cover size — dominates a tiny cover, vanishes on a realistic basin. The matrix note now states its timings are the warm median.
If you did want a literal per-row cold column in the matrix instead, I can do it, but it would have to spawn one subprocess per cell (12 methods × 4 cases = a much slower regen) and most rows would just re-show the same ~1× — flagging the tradeoff rather than assuming. Generated by Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #131 +/- ##
==========================================
+ Coverage 94.38% 94.60% +0.21%
==========================================
Files 9 9
Lines 1318 1371 +53
==========================================
+ Hits 1244 1297 +53
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:
|
2 participants
Closes #65
Refs #48
What / approach
Per @espg's decisions on the issue (2026-07-22):
docs/with a README pointer (README stays lean).geo2mort) and decode (mort2geo).main.New self-writing generator
bench_cross_order.pymirrors the existingbench_matrix.pyprecedent: fixed-seed inputs, prints a markdown table and rewrites it in place between<!-- BENCH_CROSS_ORDER:START/END -->markers into a newdocs/benchmarks.mdpage. Columns: encode (M idx/s), decode (M idx/s), coverage (cells / ms) at orders {4, 12, 18, 29}.Sample of the generated table (real numbers from a run on the built extension in this worktree — timings are machine/run dependent):
Coverage scoping (said out loud, not silently capped): flat
morton_coveragescales as ~4**order along the boundary and exhausts memory well before order 29 (measured: 78.5M cells at order 18 on a 2° box). The cross-order coverage column therefore uses the compactmorton_coverage_mocon a small fixed ~0.01° (~1 km) box, deliberately chosen so order 29 stays tractable, and the doc/script both explain this and point todocs/coverage_methods.mdfor the flat-vs-MOC / budget trade-offs on real polygons.Phases
bench_cross_order.py+docs/benchmarks.md(table page).docs/benchmarks.md; regenerated thedocs/coverage_methods.mdmatrix viabench_matrix.py(timings refreshed; cell counts byte-identical, as expected).docs/benchmarks.mddocumenting the two one-line commands (mirrors howbench_matrix.pyis documented).How tested
maturin develop --release) before measuring; all numbers are real measured output, none fabricated.flake8 mortie --select=E9,F63,F7,F82clean;bench_cross_order.pyclean under--selectand--max-line-length=88.docs/benchmarks.md,docs/coverage_methods.md).pytest -v: 708 passed, 11 skipped, 1 pre-existing failure unrelated to this PR — see status comment. This PR changes nomortie/source (diff is README + docs + one new root-level bench script no test imports), so it introduces no test regression.Questions for review
benchmarks/bench_cross_order.py, but its structural twinbench_matrix.pyactually sits at the repo root (run aspython bench_matrix.py, writes intodocs/); thebenchmarks/dir holds CodSpeed/pytest bench tests, not marker-writing generators. I placed the new generator at root besidebench_matrix.pyto match the real precedent. Happy to move it tobenchmarks/if you prefer.morton_coverage_moc(compact) rather than flatmorton_coverage, since flat is intractable past ~order 15. Reasonable? (flat vs MOC is already covered indocs/coverage_methods.md.)bench_matrix.pyhas no accompanying test — it's a dev script — so I mirrored that (no behavioral test forbench_cross_order.py). Say the word if you'd like a marker-regeneration smoke test.benchmarks/test_bench_cpu.pyso CodSpeed tracks the cross-order dimension too. Out of scope for this PR unless you want it — flagging so it doesn't get lost.