Unify order2res Earth model with the spec-page sphere (R=6371.0088)#126
Conversation
There was a problem hiding this comment.
Import block is un-sorted or un-formatted
mortie/mortie/tests/test_tools.py
Lines 14 to 20 in dbba806
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of the #119 unification (order2res → exact HEALPix sphere sqrt(4πR²/(12·4^order)), R = EARTH_RADIUS_KM = 6371.0088 km). The change is correct and meets the issue's acceptance criteria. Two comments below, both non-blocking (one test suggestion, one out-of-scope observation).
Verified correct
- Formula & spec-page parity.
order2resnow returnssqrt(4πR²/(12·4^order))and matches the spec-page "cell scale" column exactly: order 0 → 6,519.623 km (table row 0), order 12 → 1.592 km (table row 12). Recomputedres0 = 6519.6235 km; ratio to the old111·58.6323 = 6508.185is 1.00176 (~0.18%), consistent with the "~0.2% larger" claim in issue #119 and §3. - Single-sourcing.
EARTH_RADIUS_KMis defined once inmortie/tools.py;test_spec_page.pynow imports it (removing its local redefinition), so code and page share one constant. No stray6371.0088literal elsewhere. - Re-pinned test values recomputed and correct.
TestRes2Display: order 12 = 1.592 km, order 13 = 795.852 m, order 25 = 19.43 cm, order 29 = 1.214 cm — all match the sphere formula.res2displaydocstring examples updated to match. These are genuine literal golden pins (not weakened) and flow throughorder2res, so they anchor the constant. - Coverage-boundary pad direction is right.
pad = pad_cells * order2res(order) / KM_PER_DEGfeeds only the "no-escape" (b) bounds —lo_bound = lat_lo - pad,hi_bound = lat_hi + pad,lon_slack = half + pad/cos_floor— so a larger pad strictly loosens those assertions (the ~0.2% increase cannot cause a spurious failure). The superset (a) check doesn't usepad. No re-pin needed there; correct to leave it. - No stale mentions missed.
58.6323/R ≈ 6366survive only indocs/specification.mdas the historical value the note explicitly says was replaced — intentional. Notebook resolution outputs render viaorder2resdynamically and their.1fdisplays (12.1 mm@order29) are unchanged (old 12.122 → new 12.144, both12.1).USAGE.md§order2resis generic ("approximate resolution in km") and stays accurate. - CLAUDE.md compliance. Commits are title-only (
phase 1 of issue #119,phase 2 of issue #119) with no attribution, matching repo style; the behavioral change carries tests in the same PR; no assertions weakened, no lint-ignore added;import mathintest_tools.pyis used.
Comments (both non-blocking): (1) test_order2res_range/_basic now pin against a helper that recomputes the identical formula/constant — self-referential, so add one literal anchor there (coverage is fine via res2display/spec_page). (2) Two near-identical Earth-radius constants now coexist in tools.py (EARTH_RADIUS_KM vs pre-existing _EARTH_RADIUS_M) — future cleanup, out of scope here.
Net: sound, ship-ready pending the optional test tweak.
Generated by Claude Code
| res = tools.order2res(order) | ||
| expected = 111 * 58.6323 * (0.5 ** order) | ||
| assert_allclose(res, expected, rtol=1e-10) | ||
| assert_allclose(res, _sphere_res(order), rtol=1e-10) |
There was a problem hiding this comment.
🤖 from Claude (review)
Self-referential pin (acceptable, but worth one literal anchor). Both test_order2res_range and test_order2res_basic now assert order2res(order) == _sphere_res(order), where _sphere_res recomputes the same formula sqrt(4πR²/(12·4^order)) with the same tools.EARTH_RADIUS_KM. This checks that order2res follows the formula, but it can no longer catch a wrong constant/formula: flip EARTH_RADIUS_KM and both sides move together, so the test stays green. Contrast the old pin (111 * 58.6323), which was an independent literal.
Coverage isn't actually lost overall — TestRes2Display pins hard literal strings that flow through order2res (1.592 km@12, 795.852 m@13, 19.43 cm@25, 1.214 cm@29 — all recomputed and correct), and test_spec_page pins the committed table literals (6,519.623 km@0), so a bad R would fail those. So this is a suggestion, not a defect: consider adding one literal assertion here (e.g. assert_allclose(tools.order2res(0), 6519.623, atol=1e-3)) so the direct order2res test is self-anchoring rather than relying on downstream tests.
Generated by Claude Code
| # Mean Earth radius (km), the exact HEALPix sphere the spec page's resolution | ||
| # table (docs/specification.md §3) derives from. order2res is the RMS cell | ||
| # spacing on this sphere; unified with the page in issue #119. | ||
| EARTH_RADIUS_KM = 6371.0088 |
There was a problem hiding this comment.
🤖 from Claude (review)
Non-blocking observation (out of scope for this diff). The module now carries two Earth-radius constants: EARTH_RADIUS_KM = 6371.0088 (here) and the pre-existing _EARTH_RADIUS_M = 6_371_008.7714 at line 752 (used by the buffer cell_width_m). These are the same physical radius to ~0.03 m (6371.0088 km = 6,371,008.8 m vs 6,371,008.7714 m). Not a defect and not introduced by #119, but a future cleanup could single-source them so the buffer path and order2res provably share one Earth model. Flagging only; no change needed in this PR.
Generated by Claude Code
Merging this PR will not alter performance
Comparing Footnotes
|
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial re-review of the order2res unification (issue #119). I tried to refute the diff and could not find a correctness or test defect — the change is sound. Verification notes:
- Formula & constant correct.
order2res(0) = sqrt(4π·6371.0088²/12) = R·sqrt(π/3) ≈ 6519.62 km, vs the old111·58.6323 = 6508.19 km→ +0.176%, matching the "~0.2% larger at every order" claim. Each order halves cleanly (area ÷4). - Re-pinned
test_tools.pyvalues check out against the sphere reference:_sphere_res(order)mirrors the implementation, and orders 12/13/25/29 reproduce theres2displaypins exactly. res2displaypins are consistent with itsround(value, 3)+str()rendering (trailing zeros dropped): order 12 →1.592 km, 13 →795.852 m, 25 →19.43 cm, 29 →1.214 cm. Docstring examples refreshed to match.test_spec_page.pynow importsEARTH_RADIUS_KMfrommortie.toolsinstead of redefining it — code and page provably share one Earth model; regenerated rows are byte-identical since the constant value is unchanged.docs/specification.md§3 note updated from "code vs. page" divergence to "code and page unified", correctly stating consumers read the cell-scale column directly.test_coverage_boundary.pycorrectly left untouched. The padpad_cells * order2res(order) / KM_PER_DEGgrows ~0.2%, which only loosens the invariant-(b) no-escape bounds (lo_bound/hi_bound/lon_slack) — no assertion is tightened or weakened in any material way (0.2% of a 3-cell pad), and the invariants still hold. Worth a one-line mention in the PR body under testing that the pad re-check was done and required no change.
Two non-blocking nits inline (docstring "Approximate" wording; float() dropping array-broadcast that the old form had, though no current caller relies on it). No changes required for correctness.
Generated by Claude Code
| def order2res(order): | ||
| res = 111 * 58.6323 * .5**order | ||
| return res | ||
| """Approximate cell scale (km) at a HEALPix tessellation ``order``. |
There was a problem hiding this comment.
🤖 from Claude (review)
Nit (wording): the summary line calls this "Approximate cell scale", but the body — correctly — says it is the exact RMS cell spacing on the equal-area HEALPix sphere (sqrt(area)). There is no approximation in the closed form; the only "approx" is that RMS spacing stands in for a per-cell shape. Consider dropping "Approximate" (or say "RMS cell scale") so the one-liner doesn't undersell the exactness the rest of the docstring establishes. Not blocking.
Generated by Claude Code
| model (issue #119). | ||
| """ | ||
| area = 4 * np.pi * EARTH_RADIUS_KM**2 / (12 * 4**order) # km2 | ||
| return float(np.sqrt(area)) |
There was a problem hiding this comment.
🤖 from Claude (review)
Minor behavioral narrowing: float(np.sqrt(area)) forces a scalar return, whereas the old 111 * 58.6323 * .5**order broadcast over an array order. I checked every consumer (res2display, test_coverage_boundary.py:78, USAGE.md, the notebooks) and all pass a scalar, so nothing breaks today — but if any caller ever passes an array of orders, float() on a non-scalar array raises TypeError. If you want to keep the previous vectorization, np.sqrt(area) (unwrapped) would return a numpy scalar for scalar input and an array for array input. Purely a note; the scalar form is fine for current usage.
Generated by Claude Code
Closes #119
What & approach
mortie.tools.order2resno longer uses the historical flat constant(
111 km/deg × 58.6323 × 0.5^order, implied sphereR ≈ 6366 km). It now derives the resolution from the exact HEALPix sphere at mean radiusR = 6371.0088 km, the same Earth model the spec page (docs/specification.md§3, PR #118) uses for both its cell-scale and cell-area columns:Because HEALPix is equal-area, every order-k cell has area
4πR²/(12·4^order); the cell scale issqrt(area), the RMS cell spacing. Code and page now share one Earth model.Behavioral change
This is a behavioral change:
order2resis ~0.2% larger at every order (order 0:6508.19 km→6519.62 km; order 29:1.212 cm→1.214 cm). Its consumers shift accordingly:res2display— per-order printout (order 12 now1.592 km, order 13795.852 m);tests/test_coverage_boundary.py— coverage-buffer padpad_cells * order2res(order) / KM_PER_DEG, now ~0.2% larger (more conservative — see Questions).Phases
EARTH_RADIUS_KMconstant + sphere-formulaorder2resinmortie/tools.py; update theres2displaydocstring examples; rewrite thedocs/specification.md§3 "code vs. page" note to reflect that code is now unified onto the page sphere.test_tools.py(test_order2res_basic/_rangenow compare against the sphere formula;test_order2res_decreasingunchanged — still valid); refreshres2displayexpected strings; switchtest_spec_page.pyto importEARTH_RADIUS_KMfrommortie.toolsinstead of redefining it locally, and update its module docstring.How tested
maturin develop --release(no Rust change here —order2resis pure Python — but the extension was rebuilt for the suite).flake8 mortie --select=E9,F63,F7,F82— clean.pytest -v— 683 passed, 11 skipped, 1 failed. The single failure is the pre-existing, unrelatedtest_spec_page.py::TestDecimalParseTieBreak::test_order29_string_parses_to_area_word(issue main is red: test_spec_page tie-break test predates #120's p-suffix rendering #123, owned by a separate PR — not touched here). Every re-pinned test passes, including the fulltest_coverage_boundary.pymatrix.Coordination note
This branch edits
mortie/tests/test_spec_page.py(theEARTH_RADIUS_KMimport + a module-docstring refresh). PRs for #123 and #124 also touch that file — flagging the potential merge-order/rebase conflict. This PR does not touch theTestDecimalParseTieBreakclass that #123 owns; the overlap is confined to the imports block and the module docstring.Questions for review
test_coverage_boundary.pygrew ~0.2% withorder2res. A larger pad only loosens the "no-escape" bounds (lo_bound/hi_bound/lon_slackare additive tolerances), so it is strictly more conservative and the whole matrix still passes — no assertion was weakened. Flagging in case you'd prefer the pad decoupled fromorder2resand pinned to a fixed km value instead._EARTH_RADIUS_M(buffer path).morton_buffer_meterskeeps its own_EARTH_RADIUS_M = 6_371_008.7714(IUGG 2015 mean, m) — that is6371.0087714 km, vs the newEARTH_RADIUS_KM = 6371.0088. They agree to ~0.03 m and Unify order2res Earth model with the spec page sphere (R=6371.0088) #119 scopes onlyorder2res, so I left the buffer constant untouched. Say the word if you'd like them collapsed to one constant.🤖 Generated with Claude Code