add HEALPix interchange guide (issue #63)#128
Conversation
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial pass on the HEALPix interchange guide. Overall this is accurate and well-scoped — I verified every claimed signature and worked example against mortie/tools.py and the section refs against docs/specification.md, and they hold up:
- API table is correct:
mort2healpix(scalar →(int, int), array →(ndarray, order)),mort2norm→(normed, base_cell, order),norm2mort(normed, parent, order)→ scalaruint64,geo2mort(lat, lon)→ length-1 array for scalar input. The[0]indexing convention is right. - The "raises on mixed orders" claim for
mort2healpixis correct —mort2normraises first (the array-path order branch inmort2healpixis effectively unreachable for mixed orders), so the guide describes real behavior. - Arithmetic checks out:
37010 == 9*4096 + 146(order 6), theint64wrap-6881500230622117885, prefix nibblebase+1, and bit-63 set for base cells 7–11. Operator precedence inint(w) >> 63 & 1parses as intended. - Argument-order pitfall (
geo2mort= (lat, lon) vs cds/healpy = (lon, lat)) is called out;healpykeying onnsideandnest=True/lonlat=Trueis correct. - healpy being outside the extras is clearly disclosed; cdshealpix correctly identified as the
test-extra oracle. Relative doc links and README link resolve correctly. Style/tables matcharrow_interchange.md.
Only minor nits, none blocking (COMMENT, not request-changes) — three inline: (1) repr() expected-outputs are numpy-2-only; (2) confirm the pinned numeric outputs (esp. the lon # 0.0) were captured from a real run so they don't drift; (3) a precision nit on "southern base cells" vs the bit-63 set 7–11.
Generated by Claude Code
| # geo2mort takes (lat, lon) and returns a length-1 array for scalar input, | ||
| # so index [0] for a single word. | ||
| m = mortie.geo2mort(-80.0, 120.0, order=6)[0] | ||
| print(repr(m)) # np.uint64(11570310392668225542) |
There was a problem hiding this comment.
🤖 from Claude (review)
The # np.uint64(...) expected-output comments here (and at line 151, print(repr(w))) are numpy-2-only. On numpy 1.x repr(np.uint64(x)) prints the bare integer (11570310392668225542), so a reader on that stack sees a "mismatch" against the guide. numpy is the sole pinned runtime dep and no floor is stated in pyproject, so both are in scope. Cheap fixes: use print(int(m)) (stable across versions) or note the outputs assume numpy ≥ 2. The scalar mort2healpix/mort2norm prints below (# 37010 6, # 146 9 6) are already version-stable since those return Python ints — only the two repr lines need it.
Generated by Claude Code
| clon, clat = healpix_to_lonlat(cell_ids, depth=order) | ||
| mlat, mlon = mortie.mort2geo(words) | ||
| print(float(np.max(np.abs(mlat - clat.to_value(u.deg))))) # ~1.98e-09 | ||
| print(float(np.max(np.abs(mlon - clon.to_value(u.deg))))) # 0.0 |
There was a problem hiding this comment.
🤖 from Claude (review)
Please confirm the pinned numeric outputs in this file were captured from an actual run, since they'll silently drift if not: the cell_ids == [...] list (line 84-85), the # ~1.98e-09 lat tolerances, and especially this # 0.0 for longitude. The lon-diff-of-exactly-zero relies on mort2geo returning longitude in [0, 360) (it does not wrap — only mort2bbox does), matching cdshealpix's [0, 360). If mort2geo ever returned [-180, 180), the eastern test points (lons 200/305/270.2) would diff by ~360 and this would not be 0.0. Worth a one-line note that these were generated from the current build (as the arrow guide implicitly does), so a future reviewer can regenerate rather than guess. Not blocking — the snippets run regardless of the comment values.
Generated by Claude Code
| base cells occupy `1..=12`, and `0` is the empty/null sentinel). Base cells | ||
| **7–11 set bit 63**, making the word a large unsigned value. Store and exchange | ||
| the word as `uint64`; reinterpreting it as signed `int64` reads southern base | ||
| cells as negative and corrupts the id. |
There was a problem hiding this comment.
🤖 from Claude (review)
Minor precision nit (optional): "reads southern base cells as negative" is slightly loose. The set that reads negative as int64 is exactly the bit-63-set cells 7–11 (stated correctly one sentence up), but base cell 7 is equatorial, not south-polar (HEALPix base cells: 0–3 north, 4–7 equatorial, 8–11 south). The int64-negative property is purely "prefix ≥ 8 ⇒ bit 63 set", independent of hemisphere — that's a different mapping from the decimal -1..-6 southern-sign in §2. Suggest "reads those base cells as negative" (or "base cells 7–11") to match the spec's own §1 wording ("base cells 7–11 set bit 63 and would read back negative"), which pointedly avoids the "southern" label here.
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 review of the HEALPix-interchange guide (issue #63). Verified every snippet against mortie/tools.py, mortie/__init__.py, pyproject.toml, and the test suite. No blocking defects — the API names, signatures, and gotchas are all correct. Notes below.
Verified correct
- All referenced functions are real and exported:
geo2mort,mort2healpix,mort2norm,norm2mort,infer_order_from_morton,mort2geo. Argument orders match:geo2mort(lat, lon, order=)is lat-first,mort2norm→(normed, parent, order),norm2mort(normed, base_cell, order). docs/healpix_interchange.md:89/:132—mlat, mlon = mortie.mort2geo(words)correctly handles mortie's unusual lat-first return order (mort2georeturnslat, lon). Easy to get wrong; got it right.- The scalar worked example (order 6 →
cell 37010, word11570310392668225542, base 9, normed 146) matches themort2healpixdocstring intools.pyand is internally consistent (9*4096+146 == 37010). - The cdshealpix block reuses the exact order-14 lat/lon arrays and
lonlat_to_healpix(lons*u.deg, lats*u.deg, depth=order)oracle call fromtest_morton_index.py::test_latlon_matches_cdshealpix_nested— a faithful mirror of the test-suite oracle pattern. - Extras claims accurate:
cdshealpix>=0.7is in thetestextra;healpyis in no extra (guide says install separately). numpy-only runtime respected (§7). - healpy calls correct:
ang2pix(nside, lons, lats, nest=True, lonlat=True)andpix2ang(..., lonlat=True)both use thenside/lonlat convention properly. - Gotcha 1 arithmetic checks out: prefix nibble
= base+1(int(w)>>60 == 10for base 9), bit 63 set, andnp.int64(w) == -6881500230622117885(=w - 2**64) is exactly the signed-misread value shown. Gotchas 2 (order self-encoding, mixed-order raise viamort2norm) and 3 (bare call → order-29 point) are accurate.
Minor (non-blocking)
docs/healpix_interchange.md:103— the "Importing a foreign nested id" block usesoracle,order, andnside_sqfrom the previous code block, so it only runs as a continuation. A one-line "(continues the previous snippet)" note, or inliningorder = 14, would keep it copy-run-safe.- Wiring: there is no docs index/toctree in the repo (
docs/is flat markdown linked ad hoc from README), so linking the guide from README is the right pattern. Notedocs/arrow_interchange.mdis not linked from README either — out of scope here, just flagging the inconsistency. - The exact array cell-ids and the
~1.98e-09/0.0tolerance comments couldn't be re-run here (review-only, no build), but they mirror the test inputs and the shared cds-healpix core, so they're plausible; worth a finalpytest/manual run before un-drafting.
Generated by Claude Code
|
this doc mentions cdshealpix as verification, but doesn't mention or track https://github.com/matt-cornell/healpix-rs -- which is the actual crate we use in production for transforms to/from NESTED and other healpix structures and wrapped code functions. please update and add. |
…cell wording (issue #63)
|
🤖 from Claude Folded the three non-blocking self-review nits in (1) The two (2) Added a one-line note after the cdshealpix round-trip block that the (3) Reworded the gotcha-1 sentence from "reinterpreting it as signed Docs-only, no build needed. Leaving the review threads for the reviewer to resolve. Generated by Claude Code |
|
🤖 from Claude Good catch — the earlier run only folded the self-review nits and missed this. Addressed in Generated by Claude Code |
|
🤖 from Claude Tacked the main-greening fix into this PR as you asked. The red on main (run 29847408029) was Two commits added:
order = np.asarray(order, dtype=np.float64)
area = 4 * np.pi * EARTH_RADIUS_KM**2 / (12 * 4.0**order) # km2
res = np.sqrt(area)
return float(res) if res.ndim == 0 else resTwo bugs, one fix: (a) Green locally (numpy 2.4.6): Kept as draft — it's yours to mark ready and merge whenever CI confirms. Left Generated by Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #128 +/- ##
==========================================
+ 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:
|
Closes #63. Refs #48.
What
Adds
docs/healpix_interchange.md, a Tier-2 v1.0 guide for moving a packedmorton_indexword to and from the wider HEALPix ecosystem, and links it fromthe README next to the specification reference.
The guide covers:
mort2healpix(word →(nested_ipix, order)),mort2norm/norm2mort(split/join aroundbase_cell * nside**2 + normed),and
infer_order_from_morton— grounded in the real signatures inmortie/tools.py.lonlat_to_healpix/healpix_to_lonlat),the same oracle the suite uses in
test_morton_index.py/test_coverage_hemisphere.py, plus importing a foreign NESTED id back into aword.
ang2pix/pix2ang,nside = 2**order,nest=True,lonlat=True).base_cell + 1and basecells 7–11 set bit 63, so words must stay
uint64(readingint64corruptssouthern cells); (2) order is self-encoded — never carry it out of band, but
every ecosystem call needs it explicitly; (3) a bare
geo2mortyields anorder-29 point, and the null sentinel is the all-zero word.
Approach
Every code block was executed against the built extension and the installed
ecosystem libs, and the pasted outputs are the real values. Snippet argument
order is called out where it differs (
geo2mortis(lat, lon); cdshealpix andhealpy are
(lon, lat)). numpy stays the only runtime dep — cdshealpix is inthe
testextra (the suite's oracle); healpy is not in any mortie extra, andthe guide says so and shows
pip install healpy.Phases
How tested
maturin develop --release(rebuilt the Rust ext), then installed thetestextra (
cdshealpix==0.8.1,astropy==8.0.1) andhealpy==1.19.0separately.cdshealpix and healpy exactly; centers agree to ~2e-9 deg).
flake8 mortie --select=E9,F63,F7,F82— clean.pytest -q— 683 passed, 11 skipped, 1 failed. The single failure is thepre-existing
test_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), unrelated to this docs-only change.
Questions for review
docs/arrow_interchange.mdcurrently isn't linked from the README at all, sothere's no single docs index to slot into — happy to move the link or add a
proper docs index if preferred.
test). Kept it as an optionalpip install healpyblock. Fine as-is, orshould the healpy snippet be dropped to keep every snippet runnable purely
from the declared extras?
🤖 Generated with Claude Code
Generated by Claude Code