Fix res2display() order cap and add km/m/cm unit ladder#102
Conversation
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Reviewed the diff for #99 against origin/main. Core fix is correct and well-grounded: the km/m/cm ladder cutoffs (>=1.0 km, >=1e-3 m, else cm), range(max_order+1) inclusivity, and round-to-3 are all right, and the golden values match the issue examples exactly (order 12 → 1.589 km, 13 → 794.456 m, 25 → 19.396 cm, 29 → 1.212 cm). Within the sanctioned range 0..29 there are no bracket-boundary or sub-1-cm surprises. Tests are a solid addition — order count, max_order bound, unit ladder, and rounding are all covered. Terse, matches surrounding style; module is 860 lines (under the ~1000 limit); no new deps. No blocking issues.
Findings (all Low / non-blocking, for @espg to fold in or wave off):
- Low — unvalidated
max_orderat both ends. Negative → silent empty output;>29→ prints beyond-kernel orders and, past ~32, sub-1 cm fractions with no mm rung, contradicting the docstring. Inline on the loop. Consider clamp-or-raise + a test. - Nit — round-after-convert can cross a bracket boundary (
0.9999996 km→1000.0 m). Not triggered by any real order; latent if the range widens. Inline. - Nit —
test_rounds_within_bracketdoesnumber.split('.')[1]which wouldIndexErrorif a value ever printed without a decimal point. Safe for all current values (Pythonround(float,3)always yields a dotted str), but brittle; a'.' in numberguard or asserting the format directly would be sturdier.
Docstring is otherwise accurate for the default. Nothing here needs to block landing the phase.
Generated by Claude Code
| naturally (e.g. order 12 -> ``1.589 km``, order 13 -> ``794.456 m``) | ||
| rather than as tiny km fractions. | ||
| ''' | ||
| for res in range(max_order + 1): |
There was a problem hiding this comment.
🤖 from Claude (review)
max_order is unvalidated at both ends. Two edge cases neither guarded nor tested:
- Negative
max_order→range(n<1)is empty, so the function silently prints nothing (e.g.res2display(-1)returns with no output, no error). Silent no-op on bad input is easy to misread as "nothing to display". max_order > 29→ happily prints beyond the kernel'sMAX_ORDER, contradicting the docstring's "29 = the finest order the kernel reaches". Worse, past order ~32 the cm bracket itself drops below 1 (0.152 cm,0.019 cm, …) — there's no mm rung, so it degrades to sub-1 cm fractions, the exact readability problem this PR set out to fix.
Suggest clamping/validating to 0..MAX_ORDER (e.g. raise ValueError on out-of-range, or min(max_order, MAX_ORDER)), and add a test pinning the chosen behavior. Not a blocker for the sanctioned scope, but the docstring currently over-promises vs. the code.
Generated by Claude Code
| value, unit = km * 1e3, 'm' | ||
| else: | ||
| value, unit = km * 1e5, 'cm' | ||
| print(str(round(value, 3)) + ' ' + unit + |
There was a problem hiding this comment.
🤖 from Claude (review)
Nit / latent: bracket selection uses the raw km, but the printed number is round(value, 3) after conversion. A value just under a boundary can round up across it and print in the smaller unit — e.g. km = 0.9999996 selects the m bracket and prints 1000.0 m instead of 1.0 km (same for 1000.0 cm). No order in 0..29 triggers this, so it's harmless today, but it's a real inconsistency if the range ever widens or order2res constants change. Worth a one-line comment or a post-round bracket re-check.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Left standing intentionally. As the review notes, no order in the sanctioned 0..MAX_ORDER range lands within rounding distance of a bracket boundary (the closest is order 12 at 1.589 km, nowhere near 1.000), so this can't fire today. A post-round bracket re-check would add branching to a print helper for a case that only becomes reachable if order2res's constants change or the range widens past 29 — both of which are now guarded against by the phase-2 ValueError. Flagging it here rather than folding it in; happy to add the re-check if you'd prefer the belt-and-suspenders version.
Generated by Claude Code
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
==========================================
+ Coverage 93.73% 93.94% +0.20%
==========================================
Files 9 9
Lines 1230 1238 +8
==========================================
+ Hits 1153 1163 +10
+ Misses 77 75 -2
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 improve performance by 12.02%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | coverage_triangle[4] |
130.1 µs | 114.4 µs | +13.77% |
| ⚡ | fast_norm2mort_orders[6] |
269.4 ns | 240.3 ns | +12.14% |
| ⚡ | fast_norm2mort_orders[10] |
315.3 ns | 286.1 ns | +10.19% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/99-res2display-order-cap (a316426) with main (4fb4cd2)2
Footnotes
-
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. ↩
-
No successful run was found on
main(5b92649) during the generation of this report, so 4fb4cd2 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Phase-2 review (commit 794b89d — validation + docstring + test_out_of_range_raises, and the '.' in number test guard). Reviewed with fresh context against origin/main.
Verdict: correct and safe. No blocking issues. Two minor, non-blocking notes inline.
What I verified:
- Boundary inclusivity — correct.
not 0 <= max_order <= MAX_ORDERaccepts 0 and 29 and rejects -1 and 30. Confirmed by executing the predicate directly. - Error message format — safe.
%donMAX_ORDER(int) and%ronmax_orderwon't raise a formatting error for any input type. - New test exercises both ends.
test_out_of_range_raisesloops(-1, MAX_ORDER + 1), hitting the just-below and just-above rejections, and pins the message viamatch="max_order must be". Acceptance of the 0/29 boundaries isn't asserted in this test, but it's covered indirectly (default run reaches 29;range()covers 0) — fine as is. '.' in numberguard converts a potentialIndexError(on a value with no decimal point) into a clean assertion. Harmless robustness improvement;str(round(float, 3))always contains a.at the orders in play, so it never fires today.- No new dependencies, no scope creep, module-size fine.
Minor notes (inline): (1) the raise is the module's only %-formatted message — an f-string matches the surrounding convention and is terser; (2) the docstring justifies the 0..MAX_ORDER cap by display legibility rather than the kernel's order-29 limit, which is the real reason.
One observation, no action needed: validation guards range but not type — a float like 2.5 slips past 0 <= 2.5 <= 29 and then fails later in range(max_order + 1) with a bare TypeError, and a str fails in the comparison itself. Out of scope for this fix; flagging only for completeness.
Generated by Claude Code
| if not 0 <= max_order <= MAX_ORDER: | ||
| raise ValueError( | ||
| 'max_order must be between 0 and %d, got %r' | ||
| % (MAX_ORDER, max_order)) |
There was a problem hiding this comment.
🤖 from Claude (review)
Validation logic is correct: 0 <= max_order <= MAX_ORDER accepts both boundaries (0 and 29) and rejects -1 / 30, and %d/%r are safe (%r reprs any type without a formatting error). No functional objection.
Minor, non-blocking — style: this is the module's only %-formatted raise. Every other interpolated message uses an f-string (e.g. f"Mixed orders in morton array: {set(...)}" at line 289, and __init__.py/arrow.py/coverage.py follow the same convention). An f-string here matches the surrounding code and collapses three lines to one:
if not 0 <= max_order <= MAX_ORDER:
raise ValueError(f"max_order must be between 0 and {MAX_ORDER}, got {max_order!r}")(coverage.py's raise ValueError("Order must be between 1 and 29") is the plain-literal precedent; nothing in the module uses %.)
Generated by Claude Code
|
|
||
| ``max_order`` must lie in 0..MAX_ORDER; the km/m/cm ladder bottoms out | ||
| at cm, which only stays legible while resolutions stay at or above the | ||
| order-29 floor. |
There was a problem hiding this comment.
🤖 from Claude (review)
Nit, non-blocking — docstring rationale: "the km/m/cm ladder bottoms out at cm, which only stays legible while resolutions stay at or above the order-29 floor" justifies the upper bound by display legibility, but the actual reason max_order caps at MAX_ORDER is that 29 is the packed-u64 kernel's max order (the same limit already documented on the MAX_ORDER constant at line 16-19). The legibility framing reads as the cause when it's really a side effect. Consider tying the cap to the kernel limit instead, and trimming — the paragraph is a touch verbose for a print helper.
Generated by Claude Code
Closes #99.
What this does
mortie/tools.py::res2display()loopedfor res in range(20), so it silently printed nothing for orders 20–29 even though the packed-u64 kernel reaches order 29 (MAX_ORDER). It also printed every order in km, so fine orders read as tiny km fractions (0.0248… km) instead of naturally.Two changes, matching the design signed off on the issue thread (#99#issuecomment-4883350921):
range(max_order + 1), defaulting toMAX_ORDER(29), sourced from the existing module constant rather than a bare literal. Added an optionalmax_order=29parameter so callers can bound it; the no-arg call stays backward compatible.1.589 kmand order 13 reads794.456 m— exactly the issue's requested output.Approach
The bracket cutoffs key off
order2res(which returns km and was already order-unbounded):round(value, 3)reproduces the issue's examples:1.588912… → 1.589,794.4562… → 794.456,19.396,1.212. The three design questions were answered on the thread: km/m/cm ladder (yes), optionalmax_order=29(yes), round to three decimals within a bracket (yes).Full rendered output for reference:
Phases
res2display, pluscapsystests intest_tools.py(order count 0–29, km/m boundary at order 12/13, cm at the fine end, ≤3-decimal rounding, and themax_orderbound).max_orderoutside0..MAX_ORDERwith aValueError(negative was silently a no-op;> 29printed sub-1 cm past the kernel floor), with a test covering both ends; hardened the rounding-test assertion against a'.'-less value.%-format raise to an f-string to match the module's uniform style, and tightened the docstring to cite the kernel's order-29 encoding limit.The per-phase adversarial self-review ran on phases 1 and 2 and converged — the remaining findings were all low-severity convention nits, now addressed in phases 2–3; phase 3 changed no logic (f-string + docstring only).
How it was tested
maturin develop --release(Rust ext rebuilt),flake8 mortie --select=E9,F63,F7,F82clean.pytest mortie/tests/test_tools.py— 46 passed (6 new inTestRes2Display). No Rust change, socargogates are untouched.res2display(); output matches the issue's requested examples byte-for-byte.Questions for review
None outstanding — the three design questions were resolved on the issue thread before implementation.