📝 docs(spec): specify chord_distance and interval - #732
Merged
nstarman merged 2 commits intoAug 18, 2026
Conversation
Of the manifold measurement verbs, `norm`, `angle_between` and `geodesic_distance` each carry a `software-spec-*` section; `chord_distance` (GalacticDynamics#720) never had one, and `interval` had eight passing mentions but no entry of its own. Both are public, both are reachable as `cx.*`, and neither was specified. `chord_distance` follows the `geodesic_distance` entry, since the two are a pair: same signature shape, opposite question. Its section records that the result carries the ambient's length unit for an `EmbeddedManifold` but is *dimensionless* on the bare `HyperSphericalManifold` -- whose canonical embedding is the unit sphere, so the chord is a pure ratio. That is a real asymmetry with `geodesic_distance`, which returns an `Angle` on the same manifold, and it is the sort of thing a spec exists to pin: an arc is an angle, a chord is not. `interval`'s section leads with what it is for -- the signed form is defined where `norm` has no real value and `geodesic_distance` refuses outright -- and then states plainly that it is **not** a distance. It is neither chart-invariant nor symmetric, and both were measured rather than asserted: same two points, flat plane: cart2d 5.0 m^2 polar2d 3.467 m^2 geodesic_distance 2.236 m in both sph2, one pair: (a,b) 0.813 rad^2 (b,a) 0.999 rad^2 So `interval == geodesic_distance**2` holds only on a flat manifold *in Cartesian coordinates*; flatness alone is not enough, since a curvilinear chart on flat space has a varying metric. Same correction GalacticDynamics#724 made to the docstrings, now stated where the contract lives. Also updates `geodesic_distance`'s "sits alongside `norm` and `angle_between` as the third manifold measurement", which stopped being complete when `chord_distance` landed, and cross-links the two. Every example was executed before being written down. `docs/spec.md` is excluded from pytest via `--ignore-glob`, so nothing in CI would have caught a wrong one -- three reprs were wrong on the first pass (`Quantity(Array(-24., ...))` rather than `Q(-24., 'm2')`) and only running them found it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
📝 Add / update documentation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds missing authoritative spec entries in docs/spec.md for two public manifold measurement verbs (chord_distance and interval), and updates the surrounding geodesic_distance spec text to cross-link the new contract.
Changes:
- Add a new
software-spec-chord_distancesection defining semantics, dispatch behavior, return types, and examples. - Add a new
software-spec-intervalsection clarifying purpose/limitations (chart dependence, asymmetry) plus signatures and examples. - Update
geodesic_distanceprose to referencechord_distanceas the paired ambient-space measurement.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #732 +/- ##
==========================================
+ Coverage 96.46% 96.56% +0.10%
==========================================
Files 266 266
Lines 8738 8852 +114
==========================================
+ Hits 8429 8548 +119
+ Misses 309 304 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review caught that the bullet asserted `norm(b - a)**2 == interval(a, b)`, which
is not executable: `CDict` is a plain mapping, so `b - a` raises `TypeError:
unsupported operand type(s) for -: 'dict' and 'dict'`, and `norm` needs the
chart and the `at=` that fixes where the metric is evaluated. Loose prose
carried over from the source comment, which is fine in a comment and not in a
normative contract.
The identity itself holds. Now stated with the component-wise `diff` and the
base point spelled out, and demonstrated on `sph3d` -- a chart whose metric
genuinely varies, so the example would fail if `at=` were dropped rather than
passing for a constant-metric reason:
norm(diff, sph3d, at=a) ** 2 -> Q(1.629626, 'm2')
interval(sph3d, a, b) -> Q(1.629626, 'm2')
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 18, 2026
nstarman
added a commit
to nstarman/coordinax
that referenced
this pull request
Aug 18, 2026
Review caught explanatory sentences sitting inside ```pycon blocks. Everything in a fence renders as code, so those lines came out monospaced and their inline markup -- ``x/y/z``, *names* -- rendered literally. Fixed by closing the fence around the prose rather than by prefixing it with `>>> #`, which would have kept it inside the block and dropped the markup entirely instead of just failing to render it. Six occurrences, not the one flagged: one is this branch's, four came in with GalacticDynamics#732, and `Points move by ``delta * tau``` predates both. Same defect and the same file, and this is the branch that puts the file under CI, so fixing the class rather than the instance belongs here. Sibling fences are fine for sybil -- the namespace is per document, so state carries across the split. (Nesting a fence inside an admonition is what raises `ValueError: <Region ...> overlaps <Region ...>`; that is a different shape and not what this does.) 300 passed, `prek run --all-files` clean, docs build succeeds.
nstarman
added a commit
to nstarman/coordinax
that referenced
this pull request
Aug 18, 2026
Review caught explanatory sentences sitting inside ```pycon blocks. Everything in a fence renders as code, so those lines came out monospaced and their inline markup -- ``x/y/z``, *names* -- rendered literally. Fixed by closing the fence around the prose rather than by prefixing it with `>>> #`, which would have kept it inside the block and dropped the markup entirely instead of just failing to render it. Six occurrences, not the one flagged: one is this branch's, four came in with GalacticDynamics#732, and `Points move by ``delta * tau``` predates both. Same defect and the same file, and this is the branch that puts the file under CI, so fixing the class rather than the instance belongs here. Sibling fences are fine for sybil -- the namespace is per document, so state carries across the split. (Nesting a fence inside an admonition is what raises `ValueError: <Region ...> overlaps <Region ...>`; that is a different shape and not what this does.) 300 passed, `prek run --all-files` clean, docs build succeeds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Of the manifold measurement verbs,
norm,angle_betweenandgeodesic_distanceeach carry asoftware-spec-*section.chord_distance(#720) never had one, andintervalhad eight passing mentions but no entry of its own. Both are public and reachable ascx.*; neither was specified.chord_distancePlaced directly after
geodesic_distance, since the two are a pair — same signature shape, opposite question.The part worth having written down is the return type:
geodesic_distancechord_distanceHyperSphericalManifold(unit sphere)AngleinradQuantityEmbeddedManifold,TwoSphereIn3D(radius=Q(2, "m"))DistanceinmDistanceinmThat asymmetry is deliberate and easy to mistake for a bug: an arc is an angle, a chord is not, and the bare manifold has no length scale, so its chord is a pure ratio. Consistent with the reasoning in #716 for the intrinsic metric being dimensionless.
intervalLeads with what it is for — the signed form is defined where
normhas no real value andgeodesic_distancerefuses outright — then states plainly that it is not a distance. Unlike the other four verbs it is neither chart-invariant nor symmetric, and I measured both rather than asserting them:So
interval == geodesic_distance**2holds only on a flat manifold in Cartesian coordinates — flatness alone is not enough, because a curvilinear chart on flat space has a varying metric. That is the same correction #724 made to the docstrings, now stated where the contract lives.Also updates
geodesic_distance's "sits alongsidenormandangle_betweenas the third manifold measurement", which stopped being complete whenchord_distancelanded, and cross-links the two.Verification
Every example was executed before being written down, because nothing in CI would catch a wrong one:
docs/spec.mdis excluded from pytest via--ignore-glob=docs/spec.md. Three reprs were wrong on the first pass —Quantity(Array(-24., dtype=float32), unit='m2')rather thanQ(-24., 'm2')— and only running them found it.uv run --frozen nox -s docs(warnings as errors): build succeeded, cross-link resolvesprek run --all-filesclean (blacken-docsreformatted the blocks; re-verified after)Found while here, deliberately not fixed
That
--ignore-globis hiding real rot. Running the file anyway:The 17 are stale outputs, mostly
weak_type=Truereprs, plus oneNameError: name 'manifold' is not defined. None are in the sections this PR adds. It looks like the exclusion could be dropped once those are fixed, which would put all 303 example lines under CI — but that is a separate change from adding two entries, and mixing them would bury this diff. Happy to open it as a follow-up.Also still unspecified:
causal_character,proper_timeandproper_distancein thelorentziansub-namespace, and the plumbing verbs (metric_matrixat 26 mentions is the largest). Left out to keep this to the two measurement verbs.🤖 Generated with Claude Code