workspace split: extract mortie-core (issue #200) - #207
Conversation
| @@ -1,3 +1,8 @@ | |||
| [workspace] | |||
| members = ["mortie-core"] | |||
There was a problem hiding this comment.
🤖 from Claude (review)
default-members defaults to the root package only, so bare cargo commands will silently skip mortie-core from phase 2 onward.
Verified on this commit:
$ cargo metadata --format-version 1 --no-deps # (fields extracted)
workspace_members: ["mortie-core#0.1.0", "mortie@0.9.9"]
workspace_default_members: ["mortie@0.9.9"]
Cargo's rule: in a non-virtual workspace (root manifest carries a [package], as here), default-members defaults to the root package alone — not "all members", which is the default only for a virtual manifest. Phase 1 hides the consequence because mortie-core is empty, but once the codec and its unit tests move in phase 2:
cargo test,cargo bench,cargo clippy— the exact commandsBUILDING.mdtells contributors to run ("Testing", and "Contributing" steps 1/3/5) — build onlymortie. The moved codec tests stop running for anyone who does not type--workspace..github/workflows/codspeed.ymlruns barecargo codspeed build/cargo codspeed run; any bench that follows the codec intomortie-coreis outside the default package selection.- The PR body's green Rust result is
cargo test --workspace(393). That flag is load-bearing, and nothing in CI or the docs supplies it — the repo has nocargo teststep at all (checked all five workflows).
Cheapest fix while the crate is still empty: add default-members = [".", "mortie-core"] to this [workspace] table (and/or make the CI and BUILDING.md invocations explicitly --workspace). Landing it with the scaffolding keeps phase 2 from being the commit where Rust coverage quietly drops.
There was a problem hiding this comment.
🤖 from Claude
Accepted and fixed in b20c327 — default-members = [".", "mortie-core"] added to the [workspace] table, with a comment recording why (non-virtual workspace, so the default is the root package alone).
Verified the selection now covers both members:
$ cargo metadata --format-version 1 --no-deps # (fields extracted)
workspace_members: ["mortie-core#0.1.0", "mortie@0.9.9"]
workspace_default_members: ["mortie@0.9.9", "mortie-core#0.1.0"]
and the root package still builds every target: cargo check --all-targets → Finished dev profile. Full local gate after the fold: cargo test --workspace 393 passed / 1 ignored plus the new mortie-core tests, and cargo bench --no-run builds all four bench executables.
You are right that this is cheapest to land now while the crate is empty — with it in place, the phase-2 move of decimal_morton/morton carries its unit tests into the default selection rather than out of it, so bare cargo test/cargo bench/cargo codspeed build keep covering the codec without anyone having to remember --workspace.
Leaving the BUILDING.md / codspeed.yml half of your suggestion alone deliberately: with default-members correct, the documented bare invocations are already right, and editing .github/workflows/ is out of bounds for this PR beyond mechanical path fixes.
| edition = "2021" | ||
| authors = ["Shane Grigsby (espg) <refuge@rocktalus.com>"] | ||
| description = "Packed-word morton codec for HEALPix grids (the mortie id codec)" | ||
| license = "MIT" |
There was a problem hiding this comment.
🤖 from Claude (review)
Publish metadata is incomplete for a crate whose stated purpose is to be published.
cargo already warns on every packaging run of this manifest — straight out of maturin sdist on this commit:
From `cargo package --list --allow-dirty --manifest-path .../mortie-core/Cargo.toml`:
warning: manifest has no documentation, homepage or repository
And the packaged file list is:
$ cargo package --list --allow-dirty --manifest-path mortie-core/Cargo.toml
.cargo_vcs_info.json
Cargo.lock
Cargo.toml
Cargo.toml.orig
src/lib.rs
So the crate declares license = "MIT" but ships no license text: the repo's LICENSE sits at the workspace root, and cargo package only collects files inside the package directory. Per the #48 ruling (comment 5339056931) this crate exists precisely so it can be a published dependency for the healpix-geo/mortie-core split, so the .crate that eventually lands on crates.io would carry a bare license identifier, no license file, and no repository link.
Concretely missing here: repository (plus homepage/documentation to silence the warning), readme, rust-version (no MSRV is declared anywhere in the repo — the root manifest has none either), and a LICENSE file under mortie-core/ (or a license-file pointing at one inside the package).
Separately: version/edition/authors/license are now duplicated between the root manifest and this one, with no shared source. A [workspace.package] table plus field.workspace = true here would keep them from drifting — worth noting that build-wheels.yml's release step rewrites only the root manifest's ^version = line, so these copies are already on a different track by construction.
There was a problem hiding this comment.
🤖 from Claude
Partially folded — the packaging warning is fixed in d77ae9e; the rest is scoped to #201.
Taken: repository = "https://github.com/espg/mortie" added to mortie-core/Cargo.toml. That is the one item whose cost is paid now, on every packaging run, and it is one line. Before:
$ cargo package --list --allow-dirty --manifest-path mortie-core/Cargo.toml
warning: manifest has no documentation, homepage or repository
.cargo_vcs_info.json
...
after: same file list, no warning.
Left standing, for #201: the license text under mortie-core/ (or license-file), readme, rust-version/MSRV, homepage/documentation, and the [workspace.package] + field.workspace = true dedup. Not a disagreement on the merits — your cargo package --list output is right that the eventual .crate would ship a bare MIT identifier with no license file, and right that the root/member copies of version/edition/authors/license have no shared source. It is a scope call: this PR is the workspace split only, with no crates.io metadata beyond what a path-dependency workspace member needs (issue #200 ruling), and crates.io publish prep is issue #201 with the licensing/provenance diligence in #202. mortie-core is a path dependency of mortie today and is not published by anything in this PR, so none of those omissions can bite before #201 lands.
One point from your comment worth carrying over to #201 verbatim, because it is a real trap: build-wheels.yml's release step rewrites only the root manifest's ^version = line, so mortie-core's version is already on a separate track by construction — whoever does #201 has to decide whether the two versions move together (version.workspace = true) or independently, before the first publish rather than after.
These stay standing for espg to route onto #201 — I have not filed or relabelled anything, since that is a side-effecting call.
| //! | ||
| //! Workspace scaffolding (issue #200, phase 1): the codec modules | ||
| //! (`decimal_morton`, `morton`) move here in phase 2. This crate is | ||
| //! contractually dependency-minimal (issue #48): no non-std dependencies, |
There was a problem hiding this comment.
🤖 from Claude (review)
The #48 dependency-minimality contract is prose-only, and no CI job ever exercises mortie-core.
I read all five workflows (build-wheels, codspeed, docs, lint, test): none runs cargo test, cargo check, or cargo clippy. The only thing that compiles Rust in CI is the maturin wheel/sdist build, which reaches mortie-core solely as a transitive dependency of mortie — and per the default-members finding on the root manifest, even a bare cargo invocation at the workspace root leaves this package out of the default selection.
So "no non-std dependencies, and never a moc-crate dependency" is enforced by nothing but this doc comment. A phase-2/3 commit can add a dependency to mortie-core/Cargo.toml and every check in the repo stays green, while the crate is contractually the external consumers' minimal dep (issue #48 comment 5339056931; the healpix-geo/mortie#48 thread).
A one-line guard is cheap enough to land with the scaffolding — verified working on this commit:
$ cargo tree -p mortie-core --edges normal --prefix none | tail -n +2 | wc -l
0
i.e. a CI step asserting that output is empty makes the contract mechanical instead of aspirational.
There was a problem hiding this comment.
🤖 from Claude
Accepted — the contract is now mechanical as of bd462d3, though enforced from inside the crate rather than from CI.
New mortie-core/tests/dep_contract.rs reads the crate's own manifest via env!("CARGO_MANIFEST_DIR") and fails if any dependency table appears. It is a std-only line scan — no TOML parser — because pulling a parser in to guard a zero-dependency contract would be the very thing the contract forbids. It matches per dot-separated segment, so [dependencies], [dev-dependencies], [build-dependencies], [dependencies.moc] and [target.'cfg(unix)'.dependencies] all trip it, and a second test pins those forms plus the non-matches ([package], a commented-out header) so the guard itself cannot silently stop guarding.
Confirmed it actually fails rather than merely passing — temporarily appending [dependencies]\nsmallvec = "1" to the manifest:
thread 'manifest_declares_no_dependencies' panicked at mortie-core/tests/dep_contract.rs:27:5:
mortie-core must stay dependency-free (issue #48), but .../mortie-core/Cargo.toml declares: ["[dependencies]"]
then reverted. Clean state passes: cargo test -p mortie-core → 2 passed; cargo clippy -p mortie-core --all-targets and cargo fmt --check clean.
On the CI step specifically: deferred to espg, not declined. Your cargo tree -p mortie-core --edges normal --prefix none | tail -n +2 | wc -l → 0 check is a good guard and I would happily wire it, but adding a workflow step is a .github/workflows/ edit, which this PR is not authorized to make beyond mechanical path fixes. Meanwhile the in-crate test covers the gap on every cargo test — and it is genuinely reachable now, because the default-members fix on the root manifest (b20c327) puts mortie-core in the default selection, so a bare cargo test runs it. Your separate observation that no workflow runs cargo test/check/clippy at all still stands as its own gap; that is a broader CI question than this PR, and stays standing for espg.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #207 +/- ##
==========================================
+ Coverage 96.54% 96.67% +0.12%
==========================================
Files 19 20 +1
Lines 2171 2283 +112
==========================================
+ Hits 2096 2207 +111
- Misses 75 76 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| NESTED cell (or an order-29 point) with its order carried intrinsically. | ||
| Source of truth in code: `src_rust/src/decimal_morton.rs` (`MAX_ORDER = 29`, | ||
| `BODY_TUPLES = 27`). | ||
| Source of truth in code: `mortie-core/src/decimal_morton.rs` (`MAX_ORDER = 29`, |
There was a problem hiding this comment.
🤖 from Claude (review)
Stale Rust-path references the move left behind (three Python docstrings + CLAUDE.md).
This hunk correctly repoints the spec at mortie-core/src/decimal_morton.rs, but the same two paths are quoted in module docstrings that this commit does not update:
mortie/morton_index.py:7—The kernel lives in Rust (src_rust/src/decimal_morton.rs); this module is themortie/arrow.py:9—(src_rust/src/decimal_morton.rs); this module only wraps them so the samemortie/orders.py:14—Rust tree's own decomposition -- this module is the Python side ofsrc_rust/src/morton.rs.
These are the only three src_rust/ docstring references that phase 2 invalidated — I grepped the rest (mortie/convert.py, rank_xy.py, dissolve.py, geometry.py, buffer.py, tests/test_toc.py, docs/specification.md:570/640/684/689, docs/api/geometry.md) and every one of those still points at a file that stayed in src_rust/, so they are correct as-is.
They are user-visible, not just internal: each is a module __doc__ (so help(mortie.orders) prints it) and all three modules are rendered by mkdocstrings (docs/api/morton_index.md, docs/api/arrow.md, docs/api/orders.md). docs/api/orders.md:5 also says the Python surface “mirrors the Rust tree (morton.rs)”, which is now a bare filename in a different crate.
Secondary, same cause: CLAUDE.md:83 still asserts “Compiled / performance code is Rust (src_rust/src/*.rs)”, and CLAUDE.md:79 describes the Rust as living “under src_rust/”. After this commit the codec — the part docs/specification.md calls the source of truth — is under mortie-core/src/, so the contributor guidance now points a reader at the wrong tree.
There was a problem hiding this comment.
🤖 from Claude
Accepted for the three Python docstrings, fixed in 892f970. Each now names the moved file plus the re-export, matching the phrasing the spec page took in c7004c1 (mortie-core/src/decimal_morton.rs … re-exported as mortie_rustie::decimal_morton):
mortie/morton_index.py:7→mortie-core/src/decimal_morton.rs, re-exported asmortie_rustie::decimal_mortonmortie/arrow.py:9→ same pairmortie/orders.py:14→mortie-core/src/morton.rs(re-exported asmortie_rustie::morton)
Re-verified with a tree-wide grep -rn "src_rust": zero remaining references to either moved file. The other 30-odd hits all point at files that stayed in src_rust/ (dissolve.rs, sphere.rs, wkb.rs, authalic.rs, toc.rs, rank_xy.rs, cell_geom.rs, buffer.rs, plus Cargo.toml's src_rust/benches/*_bench.rs paths, which are correct — the benches did not move).
CLAUDE.md:79/:83 is left standing for espg. It is the repo instruction file, out of bounds for me to edit; :83 is the line that says compiled code lives in src_rust/src/*.rs, which is now true of everything except the two moved codec files.
Gates after the fix: python -m pytest -q 1655 passed / 16 skipped; numpydoc lint mortie/*.py clean (the hard gate the lint workflow runs); ruff check clean on the three touched files. The pre-existing ruff check / ruff format --check findings elsewhere in mortie/ are untouched by this change.
| //! grammar and its encode/decode, order/truncation/containment arithmetic, | ||
| //! the decimal-string grammar, and the `(depth, nested-ipix) ↔ packed-word` | ||
| //! pivot primitives. It is contractually dependency-minimal — no non-std | ||
| //! dependencies (enforced by `tests/dep_contract.rs`) — and it must never |
There was a problem hiding this comment.
🤖 from Claude (review)
“enforced by tests/dep_contract.rs” overstates the enforcement: nothing in CI runs cargo test.
The guard itself is sound — I read it and it is not circumventable by anything in this diff (it line-scans mortie-core/Cargo.toml for any table path segment in {dependencies, dev-dependencies, build-dependencies}, catches [dependencies.moc] and [target.'cfg(unix)'.dependencies], and the_scan_recognizes_the_forms_a_dependency_can_take guards the scanner). cargo tree -p mortie-core prints a single node, so the contract holds today.
The problem is the word enforced, which this commit newly publishes in the crate docs. grep -rn "cargo test\|cargo clippy\|cargo fmt" .github/ returns nothing:
test.yml— flake8 +pytestonlylint.yml— ruff + numpydoc, Python onlybuild-wheels.yml— maturin build/sdistcodspeed.yml—cargo codspeed build/run(benches, not#[test]s)
So a future PR that adds [dependencies] to mortie-core/Cargo.toml breaks nothing any CI job observes; the test only trips for someone who happens to run cargo test locally. That also means the whole 393-test cargo suite is unguarded, which is pre-existing and out of scope here — but the #48 contract is the one thing this PR promises is mechanical, and a doc line asserting it is the weakest link in the chain.
Two ways out, either fine: add a small cargo test job (or at minimum cargo test -p mortie-core) to test.yml, or soften this line to say the contract is checked by tests/dep_contract.rs and note it runs locally. If a CI job is out of scope for this PR, it is worth a Questions for review entry rather than leaving the stronger claim standing.
There was a problem hiding this comment.
🤖 from Claude
Accepted, softened in dd8e075. The crate doc no longer says "enforced"; it now states exactly what is true:
//! dependencies — and it must never grow a moc-crate dependency; set-ops/
//! RangeMOC layers belong elsewhere. `tests/dep_contract.rs` guards the
//! zero-dependency half by failing `cargo test` if this crate's manifest
//! declares any dependency table; no CI job runs `cargo test` today, so the
//! guard fires for whoever runs the suite rather than on every push.
Confirmed the gap the finding describes: grep -rn cargo .github/workflows/ returns only codspeed.yml (cargo install cargo-codspeed, cargo codspeed build, cargo codspeed run) — benchmark compilation, not the test suite. No workflow runs cargo test, cargo fmt, or cargo clippy.
I took the wording option rather than the CI-job option deliberately. Adding a cargo test job means editing .github/workflows/, and the project rule is not to modify CI config unless the issue names it — issue #200 is the workspace split and does not. So that half is out of scope for this PR rather than declined on the merits: the guard genuinely should run in CI, and it is a pre-existing gap this PR did not create (no Rust test job existed before the split either, it just newly matters now that a contract depends on one).
Raising the CI question for espg in the PR body so it is not lost — the natural follow-up is a Rust-checks workflow covering cargo test / cargo fmt --check / cargo clippy across the workspace, filed as its own issue.
Gates run locally on the current head: cargo fmt --check clean; workspace cargo test 393 passed (327 mortie-rustie + 66 mortie-core) plus the 2 dep_contract tests, 0 failed; cargo test -p mortie-core 66 + 2.
| // (orders 1..=27) and the 28/29 tail: all-zero, all-three, an | ||
| // alternating pattern, and a pseudo-random one. | ||
| let span = 1u64 << (2 * depth); // cells below `base` at this depth | ||
| for within in [0, span - 1, 0b01_10_11_00_01 % span, 0x2f5b_a731 % span] { |
There was a problem hiding this comment.
🤖 from Claude (review)
The sweep does not deliver the coverage the comment above it claims, and the gap lands exactly on the 28/29 tail it says it targets.
Enumerating the four within values for every depth:
- depths 0–3 collapse:
[0, span-1, 433 % span, 0x2f5ba731 % span]yields 1 distinct value at depth 0 and 3 at depths 1–3 — the two patterned constants share their low 6 bits (433 & 63 == 0x31 == 49), so at depths 1–3 they are literally the same path. - the patterned constants are small:
0b01_10_11_00_01is 9 bits and0x2f5b_a731is 30 bits, so from depth 15 up they stop wrapping and their high tuples are all zero. At depth 29 that is orders 1..=24 and 1..=14 respectively, and the other two paths (all-zero / all-three) are self-symmetric — so no path ever puts a mixed pattern in the high body at deep orders. - the tail never sees stored value 2: over the whole sweep, depth-29
(t28, t29)takes only{(0,0), (0,1), (3,3)}of 16, and depth-28t28only{0,1,3}. The point test at line 84 adds(3,0). So2— the value that separates thet28 * 5area block from thet28 * 4point block inbuild_suffix/build_point_suffix/decode_tail— is never encoded at order 28 or 29 anywhere in this file.
Mutation-confirmed. I injected a bug that mis-encodes a stored tail tuple of 2 (let t28 = if t28 & 3 == 2 { 1 } else { t28 }; at the top of build_suffix and build_point_suffix, same for t29) and reverted it afterwards:
cargo test -p mortie-core --test pivot_surface -> 4 passed; 0 failed
cargo test -p mortie-core --lib -> 10 FAILED
The inline suites catch it ten times over (order_28_suffix_form, order_29_suffix_form, nested_round_trip_all_orders, point_round_trip_and_flag, zorder_across_27_28_29_seam, both morton::tests round trips, …); this sweep does not notice. A genuinely tail-swapping bug is caught — swapping the 28/29 arms of from_nested's (t28, t29) match failed 3 of the 4 tests here — so the file is not inert, it is just narrower than advertised.
Worth noting the redundancy angle too: decimal_morton.rs's inline nested_round_trip_all_orders already runs the same all-base × all-depth round trip, and its sample_tuples cycles 0,1,2,3 across positions, so it strictly dominates this sweep on coverage. As written, the_crate_root_pivot_round_trips_every_depth_and_base_cell is 1,440 iterations of weaker duplicate behavior work in a file whose own header says behavior is pinned by the inline suites while this file pins reachability.
Two ways out, either fine: (a) derive the paths from the tuples instead of truncating constants — build within from a per-order cycle like the inline sample_tuples (tuple[i] = (seed + i) % 4) plus a couple of fixed extremes, which restores all four values at every order including 28/29; or (b) keep the sweep cheap and make the comment honest about what it does (all-zero, all-ones, and two low-bit patterns), leaning on the inline suites for value coverage.
There was a problem hiding this comment.
🤖 from Claude
Folded in 21b5ecf. The sweep is rebuilt around what the pivot actually needs, and the comments now describe exactly that.
- The four ad-hoc
withinconstants are gone. The body is now one cycling path per(depth, base): ordernstores(n + base) % 4, so a single path walks0,1,2,3across consecutive orders and, at any fixed order, the twelve base cells between them place all four stored values. That is 360 entries instead of 1,440 — the inline suites own exhaustive per-order behavior, so the external view keeps one representative path. - The 28/29 tail is enumerated rather than sampled, because the cycling paths only ever reach the diagonal
(t28, t28+1): all sixteen(t28, t29)pairs at depth 29, plus all fourt28values at depth 28 (an order-28 word stores no order-29 tuple).a_max_encoded_point_pivots_to_the_same_nested_cell_as_its_area_twinlikewise walks all sixteen pairs on every base cell, sincebuild_point_suffixkeys off the pair directly rather than the area tail's order code.
Mutation-sensitivity, measured here with your mutation (mapping stored tail value 2 to 1 in both build_suffix and build_point_suffix):
- before (
fc81d99's file):cargo test -p mortie-core --test pivot_surface→ 4 passed, 0 failed (the gap you found) - after (21b5ecf): 1 passed, 3 failed —
the_crate_root_pivot_round_trips_every_depth_and_base_cell,the_morton_bridge_is_the_same_pivot_in_tuple_order, anda_max_encoded_point_pivots_to_the_same_nested_cell_as_its_area_twin, e.g.left: (200065790505305797, 28)vsright: (…798, 28)
The mutation was reverted with git checkout -- mortie-core/src/decimal_morton.rs before committing; git status shows only mortie-core/tests/pivot_surface.rs in each of the two fold commits. Gates: cargo test -p mortie-core (66 + 2 + 4), workspace cargo test (327 mortie_rustie + 66 + 2 + 4), cargo fmt --all --check, cargo clippy -p mortie-core --all-targets clean, and cargo tree -p mortie-core still a single zero-dep node.
| // coarsen: truncation in packed space equals truncation in nested space. | ||
| let (order, nested) = dm::to_nested(word).expect("to_nested"); | ||
| let coarse = dm::coarsen(word, 2).expect("coarsen a valid word"); | ||
| assert_eq!( | ||
| dm::to_nested(coarse), | ||
| Some((2, nested >> (2 * (order - 2) as u32))) | ||
| ); |
There was a problem hiding this comment.
🤖 from Claude (review)
This coarsen expectation is computed by reading both sides back through to_nested, which discards exactly the property coarsen exists to maintain. to_nested only reads the top order tuples of the body, so any junk coarsen fails to zero-fill below the target order is invisible here — and canonicality (bit-equality of two encodings of the same cell) is the documented contract of the packed word.
Mutation-confirmed, then reverted: coarsen's let kept_body_orders = k.min(BODY_TUPLES); → (k + 1).min(BODY_TUPLES) keeps one tuple too many, so the result is no longer canonical.
cargo test -p mortie-core --test pivot_surface -> 4 passed; 0 failed
cargo test -p mortie-core --lib -> 3 FAILED (coarsen_zero_fills_below_target,
coarsen_matches_reencode,
coarsen_point_below_28_becomes_area)
The packed-word form is one line shorter, needs no order/nested locals, and does catch it:
assert_eq!(dm::coarsen(word, 2), Some(dm::encode(4, &tuples, 2)));I verified that holds on unmutated main of this branch. It also matches the style of the common_ancestor assertion six lines below, which already states its expectation as a packed word (Ok(dm::encode(4, &tuples, 4))) rather than routing through the decoder — so this is an internal inconsistency as much as a weak assertion.
Same note, smaller stakes, for the assertion's comment: "truncation in packed space equals truncation in nested space" is a claim about the two spaces agreeing, but with the expectation derived from to_nested(word) the assertion is closer to self-consistency than to an independent check.
There was a problem hiding this comment.
🤖 from Claude
Applied verbatim in 688cd86:
// coarsen: truncating to order 2 is bit-identical to encoding the first two
// tuples at order 2. Stated as a packed word rather than read back through
// `to_nested`, which only inspects the top `order` tuples and so cannot see
// non-canonical leftover bits below the target order.
assert_eq!(dm::coarsen(word, 2), Some(dm::encode(4, &tuples, 2)));The now-unused let (order, nested) = dm::to_nested(word) binding went with it, and the assertion reads the same way as the common_ancestor one below it.
Mutation-sensitivity, measured with your mutation (kept_body_orders = (k + 1).min(BODY_TUPLES) in coarsen):
- before (
fc81d99's file):cargo test -p mortie-core --test pivot_surface→ 4 passed, 0 failed - after (688cd86): 3 passed, 1 failed —
the_documented_codec_surface_is_reachable_from_outside
Source restored with git checkout -- mortie-core/src/decimal_morton.rs before the commit; only mortie-core/tests/pivot_surface.rs is staged.
| cargo test -p mortie-core | ||
| ``` | ||
|
|
||
| On macOS, `cargo test` on the root package needs the extension-module symbols |
There was a problem hiding this comment.
🤖 from Claude (review)
Scoping check on this paragraph. The failure itself is real and the command is right — I reproduced both here on arm64 macOS: plain cargo test dies with ld: symbol(s) not found for architecture arm64 (__Py_IncRef, __Py_NoneStruct, __Py_TrueStruct, ...), and RUSTFLAGS="-C link-arg=-undefined -C link-arg=dynamic_lookup" cargo test then goes fully green across both members (root lib + descent_stats-gated harness skipped, mortie-core's dep_contract 2/2 and pivot_surface 4/4). cargo test -p mortie-core with no flag also passes on its own, so line 104 checks out.
What I would push on is the On macOS scoping against the reason given immediately after it. pyo3's extension-module feature deliberately skips linking libpython is platform-neutral — it is the same condition pyo3's FAQ describes as breaking cargo test on Linux (undefined reference to PyBaseObject_Type), where -undefined dynamic_lookup is a Mach-O-only linker flag and does nothing. And cargo test is never run in CI (no workflow under .github/workflows/ invokes it — consistent with the note in mortie-core/src/lib.rs), so the only consumer of this instruction is a human at a checkout. A Linux one copies the unqualified block at lines 89-90 (# Both workspace members / cargo test), gets a wall of linker errors, and then finds the remedy scoped to a platform they are not on.
Either fix is cheap: (a) drop On macOS, state the cause platform-neutrally, and give both remedies (macOS -undefined dynamic_lookup; Linux -C link-arg=-Wl,--unresolved-symbols=ignore-all, or link libpython), or (b) keep it macOS-scoped but say in one clause what Linux/Windows need — if they genuinely need nothing, that is worth asserting rather than leaving inferred. Either way, tagging the lines 89-90 block itself ("see the linker note below") would stop the failing command being the first thing copy-pasted.
There was a problem hiding this comment.
🤖 from Claude
Fixed in bdc399e. The note now states the cause platform-neutrally and labels the remedy as the macOS one:
cargo teston the root package needs the extension-module symbols resolved lazily on every platform, since pyo3'sextension-modulefeature deliberately skips linking libpython. The invocation below is the macOS remedy only:-undefined dynamic_lookupis a Mach-O linker option, so it does nothing on Linux or Windows — those need their own equivalent, which is not documented here because it has not been verified against this workspace.
The block itself is now commented # macOS, and the closing line is widened to cargo test -p mortie-core needs no flag on any platform — the codec crate links nothing. No substitute Linux/Windows command was added, since none could be verified on this machine.
| The Rust side is a cargo workspace with two members. `mortie-core` holds the | ||
| packed-word codec — the bit layout, encode/decode, the order arithmetic, the | ||
| decimal-string grammar, and the `(depth, nested-ipix) ↔ packed-word` pivot — and | ||
| carries **no dependencies at all**, so a Rust project can take the codec without |
There was a problem hiding this comment.
🤖 from Claude (review)
The zero-dependency claim is accurate — mortie-core/Cargo.toml has no dependency table of any kind, Cargo.lock lists mortie-core 0.1.0 with no deps under it, and cargo test -p mortie-core builds and passes with nothing else compiled. No objection there.
The claim I would tighten is the consequence: so a Rust project can take the codec (and BUILDING.md line 39, so a Rust project can depend on the codec alone). That is the stated point of the split — issue #200's consumer is an external crate — but neither page says how, and today there is no route a reader can act on. cargo search mortie returns neither mortie nor mortie-core on crates.io, and the only declaration anywhere in the tree is the root manifest's mortie-core = { path = "mortie-core" } — a path dep with no version key, i.e. local-only. Someone who reads this sentence and runs cargo add mortie-core gets nothing.
One line under the claim closes it, e.g.
mortie-core = { git = "https://github.com/espg/mortie" } # not yet published to crates.ioor, if a crates.io publish is planned as part of #200, just saying "not yet published; use a git dependency" tells the reader the git form is the interim rather than the intent. Docs-only — it just stops the sentence promising a dependency edge that cannot be written yet.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 9fdc87c. Both sites — README.md after the Rust-crates paragraph and BUILDING.md after the no-dependencies contract paragraph — now carry the same clause:
Neither crate is published to crates.io yet, so
cargo add mortie-corefinds nothing today; the crate is only available from this repository. Publishingmortie-coreis tracked separately in issue #201.
No git-dependency snippet was offered, since resolution was not verified here. No manifest was touched: no crates.io metadata, no version key, no publish wiring — that stays with issue #201.
| inheriting pyo3, numpy, rayon or a HEALPix crate. The root package `mortie` | ||
| (library `mortie_rustie`) is the pyo3 extension this Python package compiles to; | ||
| it depends on `mortie-core` and re-exports it, so every Rust path is unchanged. | ||
| See [BUILDING.md](BUILDING.md#workspace-layout) for the layout. |
There was a problem hiding this comment.
🤖 from Claude (review)
The anchor resolves — GitHub slugs ## Workspace Layout to workspace-layout, so BUILDING.md#workspace-layout is correct as written on github.com, and I confirmed neither README.md nor BUILDING.md sits in mkdocs.yml's nav (mkdocs only reads docs/), so a relative link here cannot break the mkdocs build --strict gate in docs.yml. Clean on both counts.
But that same fact is the gap: this section is invisible on the published site. docs/index.md carries a near-duplicate Installation block (pip install mortie, then "For development builds from source, see BUILDING.md") which phase 4 did not touch, so a Rust consumer landing on espg.github.io/mortie finds no mention of mortie-core anywhere except one incidental path in docs/specification.md section 1 (Source of truth in code: mortie-core/src/decimal_morton.rs). Two consequences: the site never advertises the split that #200 exists to enable, and the two Installation blocks — already near-copies — now differ, which is the shape drift takes.
One sentence in docs/index.md pointing at the same anchor (absolute URL, matching what that file already does for BUILDING.md) closes both without adding a third copy of the prose.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 9e8ad86. docs/index.md gains one paragraph, directly under the existing Installation block:
The Rust side is a cargo workspace: the root package builds the pyo3 extension, and its
mortie-coremember is the dependency-free packed-word codec. See BUILDING.md for the layout and the current crates.io status.
Link mechanics follow the pattern already in that file — the Installation block links BUILDING.md by absolute https://github.com/espg/mortie/blob/main/... URL rather than a relative path, so nothing resolves outside docs/. Deferring the layout table and the crates.io status to BUILDING.md keeps the site from re-stating detail that could drift; the one duplicated fact is the workspace/mortie-core split itself.
No nav entry, no restructuring. uv run mkdocs build --strict passes locally (mkdocs installed via uv sync --group docs, mirroring docs.yml).
|
🤖 from Claude (review) Fresh-context adversarial review of
Gates run (all green, worktree left clean):
Verified and deliberately cleared:
|
|
🤖 from Claude All four phases are landed and every adversarial-review finding is folded. The PR body checklist and "How it was tested" are updated to head ( Phases — Local gate results at
CI is stalled on runner availability, not on this PR. The last workflow job to start anywhere in this repo was created at 08:39 UTC; everything created from ~09:15 UTC onward is Left as a draft rather than flipped to ready-for-review, because the ready flip is conditioned on green CI and CI has not run on head. It should go green on its own once runners free up — no push is needed, and I've left the PR ready to flip. Two things standing for you, both in the PR body's "Questions for review":
One more item that stayed standing from the phase-2 review: |
|
🤖 from Claude Label correction: removed |
Merging this PR will regress 1 benchmark
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | coverage_square[4] |
152.4 µs | 170.3 µs | -10.47% |
| ⚡ | fast_norm2mort_orders[6] |
298.6 ns | 269.4 ns | +10.82% |
| ⚡ | fast_norm2mort_orders[10] |
315.3 ns | 286.1 ns | +10.19% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/200-workspace-split (9e8ad86) with main (2612e86)
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. ↩
|
🤖 from Claude The one red check is CodSpeed's analysis gate. I investigated the mechanism rather than dismissing it — the crate split does not regress performance, and here is the evidence. The concern worth taking seriously: this PR moves Three findings close it:
The lone regression here, Correctness is unaffected regardless: Left to @espg (hence |
1 participant
Closes #200. Refs #204 and the #48 boundary ruling (#48 (comment)):
mortie-coreis the codec only, contractually dependency-minimal, and never grows a moc-crate dependency. Full plan with the recon-verified codec boundary: #200 (comment)What this does
Converts the repo to a cargo workspace and extracts the packed-word codec into a new
mortie-corecrate (decimal_mortonminus its rayonbatchsubmodule, plus themortonpivot bridge), withmortie_rustiere-importing everything so no Rust path, bench, or Python-visible symbol changes. The(depth, nested-ipix) ↔ packed-wordpivot primitives (from_nested/from_nested_point/to_nested, andnested2mort/mort2nested) are the crate's documented public hinge, per #200 (comment) — they're what healpix-geo composes zuniq↔morton conversions through.This PR is only the workspace split: no publishing, no tags, no crates.io metadata beyond name/version/license/description/repository on the workspace member (#201/#202/#203 handle the crates.io track).
Phases
[workspace]on the root manifest (with the standalonemortie/tests/generate_geodesy_oraclehelper excluded), stubmortie-coremember, path dependency wired. Nothing moves; builds byte-identically. (fac6720, foldsb20c327d77ae9ebd462d3)mortie-core;mortie_rustiere-imports via a shim module (pub use mortie_core::decimal_morton::*+ the unmovedbatchsubmodule;pub use mortie_core::morton). Zero behavior change. (c7004c1, folds892f970dd8e075)mortie-core/tests/pivot_surface.rs);cargo test -p mortie-coregreen with an empty dep tree. (fc81d99, folds21b5ecf688cd86)docs/index.mdnote; rustdoc on the new public surface (and the crate's one rustdoc warning cleared). (2bd50c7, foldsbdc399e9fdc87c9e8ad86)What moved, and what did not
mortie-coremortie_rustiedecimal_morton.rs— the packed-word grammar,encode/encode_point/decode, order/truncation/containment arithmetic (order_of,kind_of,base_cell_of,coarsen,common_ancestor), the decimal-string grammar, the legacy converter, and the pivot primitivesdecimal_morton/batch.rs(rayon),prefix_trie.rs,geo2mort.rs,coverage*,moc*,toc.rs,sphere*,dissolve*,wkb*,buffer.rs,cell_geom.rs,linestring.rs,rank_xy.rs,authalic.rs,arrow_ffi.rs, all oflib.rs(pyo3/numpy)morton.rs— the(nested, depth)-tuple-order pivot bridgeOne test moved rather than being deleted:
from_nested_agrees_with_healpix_crateused thehealpixcrate as an oracle, whichmortie-coremay not depend on, so it now lives insrc_rust/src/geo2mort.rs's tests with a breadcrumb left where it was.cargo tree -p mortie-coreis a single line:and
mortie-core/tests/dep_contract.rsfails the suite if the manifest ever grows a dependency table, so the #48 contract is mechanical rather than prose.mortie-core/tests/pivot_surface.rsreaches the whole documented surface the way a downstream crate does (an integration test compiles as its own crate), so a visibility slip breaks the build here instead of silently breaking healpix-geo after publication; its sweep covers every stored tuple value and all 16(t28,t29)tail combinations.CI / build wiring
No workflow edits made or needed: maturin builds from pyproject + the root manifest and includes the in-tree path dependency in the sdist (verified — the built sdist contains
mortie-core/{Cargo.toml,src,tests}, and a wheel builds from the extracted sdist); the tag-syncsedinbuild-wheels.ymltouches only the rootCargo.toml(mortie-core's version is independent);cargo codspeed build/runat the workspace root still covers the three root-package benches (mortie-core has none).default-members = [".", "mortie-core"]on the root manifest keeps barecargo test/cargo benchcovering both members, since a non-virtual workspace would otherwise default to the root package alone.How it was tested
Every phase, on each commit:
cargo test(workspace): 399 passed, 1 ignored at head — 327mortie_rustie+ 66mortie-corelib + 2dep_contract+ 4pivot_surface. Phase-1 baseline was 393; the +6 are the new integration tests, and the codec's 66 are unchanged in count and content by the move.cargo test -p mortie-corestandalone: 72 passed, no flags needed (the codec crate links nothing).cargo tree -p mortie-core: one line, zero dependencies.cargo bench --no-run: all three benches compile.cargo fmt --all --check,cargo clippy -p mortie-core --all-targets: clean. (cargo clippyon the root package has 7 pre-existing warnings in files this PR doesn't touch.)cargo doc -p mortie-core --no-deps: zero warnings.mkdocs build --strict: clean.maturin develop --release+ fullpytest: 1655 passed, 16 skipped — identical to the phase-1 baseline. No Python-visible behavior or symbol change.RUSTFLAGS="-C link-arg=-undefined -C link-arg=dynamic_lookup"to link (pyo3extension-moduleskips libpython linkage). This is pre-existing — verified identical on unmodifiedmain— untouched by this PR, and now written down in BUILDING.md.Each phase went through a fresh-context adversarial review and a fold pass: 3 findings on phase 1, 2 on phase 2, 2 on phase 3, 3 on phase 4 — all folded, one commit each, every inline thread answered.
Questions for review
mortie-core/sits at the repo root, sibling ofmortie/andsrc_rust/, since it's headed for standalone extraction in publish mortie-core to crates.io: naming, semver posture, release mechanics #201. One-commit move if you'd rather it live undersrc_rust/.mortie-corestarts at0.1.0, decoupled from the Python package's tag-synced version; its 1.0.0 is gated on the healpix-geo dep PR (healpix-geo dep PR: expose the mortie-core codec through their core + wasm bindings (staged, espg posts) #203), not mortie's own 1.0 tag.from_nested,to_nested) is re-exported at the crate root; everything else, includingfrom_nested_pointand themortontuple-order bridge, stays undermortie_core::decimal_morton::/::morton::. Happy to flatten more (or none) if you have a preference for the healpix-geo dep PR: expose the mortie-core codec through their core + wasm bindings (staged, espg posts) #203 dep surface.cargo test(raised, not acted on): the phase-2 review pointed out thatmortie-core/tests/dep_contract.rs— and nowpivot_surface.rs— only fire for whoever runs the suite locally.grep -rn cargo .github/workflows/finds onlycargo install cargo-codspeedandcargo codspeed build/run; there has never been a Rust test job in this repo, so this predates the split, but the split is what makes it load-bearing (the dependency contract is the thing healpix-geo relies on). A follow-up Rust-checks workflow coveringcargo test/cargo fmt --check/cargo clippyacross both members would close it. Not done here: workflow changes are out of scope for an issue that doesn't name them, and the crate docs were reworded to state the guard's real reach rather than claim CI enforcement.