freeze path_grouping remainder rule in spec §6.1#127
Conversation
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial review of the §6.1 path_grouping remainder freeze against issue #124's acceptance and CLAUDE.md. Net: the contract is correct and the drift-pin is honest — no blocking findings. Details below.
Digit-math verification (checked against the real code path): CORRECT.
- Full id
433142241, order 8,path_grouping: 3. {sign+base}=4(leading[1-4], no sign), stands alone.- Order-digits =
33142241(8 digits = order 8). ✓ - Chunk
3+3+2→331/422/41; concatenation331+422+41=33142241. ✓ - Leaf
433142241=4+33142241. ✓ - Rendered path
4/331/422/41/433142241.zarrmatches. ✓ - I traced
MortonIndexArray.from_hive_path(["433142241.zarr"]).hive_path()[0]→4/3/3/1/4/2/2/4/1/433142241.zarr, so the skeleton test's{sign+base}, per-order digit sequence, and full-id leaf all match real code. The.zarrleaf is passed throughfrom_hive_path(suffix-stripped, single-component → digit-chain check skipped) and round-trips cleanly.
Drift-pin is non-hollow (not vacuous).
_doc_exampleasserts both markers are present ("example markers missing"), so deleting the markers fails loudly rather than silently no-op'ing — the specific failure mode to watch for is handled.test_example_skeleton_matches_hive_pathpins the leaf/sign-base/order-digit concatenation to whathive_pathactually emits, so an incoherent edit to the example fails.- The concatenation check alone is order-invariant (remainder-first
4/33/142/241/...would also concat to33142241), buttest_example_split_is_remainder_lastcloses that gap by pinning the widths to[3,3,2]— flipping remainder-first yields[2,3,3]and fails. The two tests are complementary and together cover the freeze. Judged honest given mortie ships onlypath_grouping: 1and has no grouping>1 surface to pin against; not overreaching, and correctly avoids inventing a grouping API.
Acceptance / convention checks — all pass.
- Rationale ("only split that keeps a coarser shard's grouped prefix a directory-prefix of its finer descendants") is sound: leading full-width groups are a function of digit position only, independent of total order, so they align across mixed shard orders; remainder-first makes leading boundaries order-dependent and produces disjoint trees. Matches moczarr's recorded justification.
- Explicitly states
{sign+base}stands alone and does not participate in grouping — both in the prose bullet and in the worked example. ✓ - Markers mirror the existing
table:order2resdrift-pin pattern (<!-- example:path_grouping:begin/end -->). ✓ - Commit is title-only (
freeze path_grouping remainder rule in spec §6.1 (issue #124)), no long body, no attribution line in the message. ✓ - No
@-mentions anywhere in the diff, PR body, or commit. ✓ - Test/prose style matches the surrounding file; line lengths within
--max-line-length=88. ✓ - Pre-existing
#123red-main failure is correctly excluded and flagged under "Questions for review" rather than "fixed." ✓
Minor (non-blocking, take-or-leave):
test_example_skeleton_matches_hive_pathunpackspath, grouping = self._doc_example()but never usesgrouping— could bepath, _ = .... Harmless (pyflakes doesn't flag F841 on tuple unpacking, and F841 isn't in the E9/F63/F7/F82 blocking select anyway); purely cosmetic.- Issue #124 phrased the ask as "one sentence + one worked example"; the bullet spans a few sentences (contract + remainder + rationale). The content is accurate and terse — noting only for literal fidelity to the wording, not a defect.
Nothing here needs to block the freeze. Answering the PR's three open questions is an @espg call, not something this review resolves.
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 §6.1 grouping-remainder freeze (issue #124). Verified against the issue's normative rule and the real hive_path code path — clean, no blocking defects.
Checks performed:
- Normative rule matches the issue exactly. Prose reads "the leading digit components are full-width and the final component carries the remainder (never remainder-first)" — verbatim to the issue's "leading components full-width, last component carries the remainder."
{sign+base}-stands-alone statement present and correct. "the{sign+base}component stands alone and never participates in grouping —path_groupingchunks only the order-digit string that follows it." Consistent with §6.1's constant-width{sign+base}and §2 notation.- Worked example is arithmetically correct and consistent with page notation.
433142241→head=1inhive_path(morton_index.py:586-588) →{sign+base}=4, eight order-digits33142241, leaf433142241.zarr. Atpath_grouping: 3:8 = 3+3+2leading-full-width →331/422/41, giving the documented4/331/422/41/433142241.zarr. Order digits all in[1-4], base4in[1-6]— both valid. - Justification matches the recorded cross-impl rationale (coarser shard's grouped prefix stays a directory-prefix of finer descendants → shared ancestors survive across mixed orders).
- Drift-pin test hits real code, not an invented API.
MortonIndexArray.from_hive_path/hive_pathboth exist (morton_index.py:563, 592) and are exported frommortie.test_example_skeleton_matches_hive_pathreconstructs the canonical grouping=1 decomposition from the live code and asserts{sign+base}, leaf, and concatenated order-digits all match — so editing the example incoherent fails.test_example_split_is_remainder_lastindependently pins[3,3,2](remainder-last); a remainder-first edit ([2,3,3]) fails. Both pass under the decomposition above. - Prose-normative scope is honestly stated: mortie ships only
path_grouping: 1(hive_pathemits one digit/level, no grouping param), so the split has no executable surface — the test pins the example's skeleton against real code and the partition shape in prose, which is the right seam.
No contradictions with §2 or §6.2. Terse and well-scoped.
Minor non-blocking nit (optional): in test_example_skeleton_matches_hive_path, grouping is unpacked from _doc_example() but unused in that method; harmless (not in the CI-blocking flake8 set), drop it if convenient.
Generated by Claude Code
Closes #124
What
The v1.0 spec page §6.1 declared
path_groupingbut never stated which component absorbs the remainder whenorder % path_grouping != 0. Three implementations (gridlook TShive.ts, moczarrgroup_digitsin espg/moczarr#17, and the pg3 fixture layout) all independently chose leading-full-width / remainder-last by convergent judgment, not by contract — a fourth implementation could legally pick remainder-first and produce disjoint trees.This PR freezes that rule as normative.
Approach
docs/specification.md§6.1 — added one normative "Grouping remainder (contract)" bullet plus one worked example (fenced, wrapped in<!-- example:path_grouping:begin/end -->drift markers matching the existingtable:order2respattern). It states:{sign+base}component stands alone and never participates in grouping —path_groupingchunks only the order-digit string;path_grouping: 3→3+3+2→4/331/422/41/433142241.zarr.mortie/tests/test_spec_page.py— addedTestPathGroupingRemainder, a drift-pin that parses the worked example out of the doc and anchors it against the realhive_pathcode path:{sign+base}component, per-order digit sequence, and full-id leaf must match exactly whatMortonIndexArray.from_hive_path(...).hive_path()produces for that id (this pins the{sign+base}-stands-alone rule and that the grouped components partition exactly mortie's order-digit string);Scope note on the drift-pin
mortie itself only ships
path_grouping: 1—hive_pathemits one digit per level and has no grouping>1 code surface. So the grouped split cannot be produced by mortie code, and the remainder placement is prose-normative (the freeze that #124 asks for). The test pins everything mortie can produce (the example's skeleton againsthive_path) and enforces the remainder-last arithmetic on the documented example, rather than adding a hollow test or inventing a new grouping API. Per the issue this is the right call: the rule is a cross-implementation contract for gridlook/moczarr, not new mortie behavior.How tested
flake8 mortie --select=E9,F63,F7,F82— clean; test file also clean at--max-line-length=88.pytest -q— 685 passed, 11 skipped, 1 failed. The single failure is the pre-existing red-maintest_spec_page.py::TestDecimalParseTieBreak::test_order29_string_parses_to_area_word(tracked as main is red: test_spec_page tie-break test predates #120's p-suffix rendering #123, fixed in a separate PR); this change does not touch that code. Both newTestPathGroupingRemaindercases pass.Questions for review
test_order29_string_parses_to_area_word), excluded above as out of scope. Flag if you'd rather I not open on red main.hive_path(grouping=1) and checked the split arithmetic, but the remainder placement has no grouping>1 code to pin against. If you'd prefer this stay purely prose (no test), say so and I'll dropTestPathGroupingRemainder.433142241(order 8,path_grouping: 3, split3+3+2) verbatim; it round-trips throughfrom_hive_path. Confirm the notation matches your intent for the frozen example.🤖 Generated with Claude Code
Generated by Claude Code