🐛 fix(spherical): return the intrinsic metric as a dimensionless QuantityMatrix by nstarman · Pull Request #716 · GalacticDynamics/coordinax

nstarman · 2026-08-14T11:50:04Z

Addresses #628 — but not the way that issue proposes, because its premise is wrong.

The two families are not the same geometric object

dimension of rad         ->  angle          (not dimensionless)

intrinsic S2             ->  [1, sin²θ]     dimensionless
embedded, radius = 1.0   ->  [1, sin²θ]     1 / rad2
embedded, radius = 2.0   ->  [4, 4sin²θ]    1 / rad2
embedded, radius = 2.0 m ->  [4, 4sin²θ]    m2 / rad2

Embedded is the induced metric: ds is an ambient length, so [g] = L²/rad². It scales as R².
Intrinsic is the angular metric. cxm.S2 has no radius parameter; distance on the unit sphere is the great-circle angle, so [ds] = rad and [g] is dimensionless.

They agree numerically at R = 1 only because the radius is 1. Since rad carries dimension angle, forcing 1/rad² onto the intrinsic metric would leave g with dimension angle⁻² on a manifold that has no length scale — dimensionally incoherent, not a convention choice. So the issue's option 1 is unavailable, and option 2 would discard the R² scaling that m²/rad² correctly records.

Units are therefore unchanged by this PR.

What was actually inconsistent: the container

Of the three intrinsic metric_matrix rules in _src/spherical/register_metric.py, two already returned a dimensionless QuantityMatrix — one carrying the comment # angles -> angles, so g is dimensionless. Only the main hypersphere rule returned a bare array. It now follows its own module.

That fixes the specific asymmetry #628 is about, which was visible in one place in the source:

expected = g_round.to_dense().matrix   # was: plain array
actual   = g_pullback.matrix.value

Adjacent lines, one unwrapped and one not. Both are QuantityMatrix now, so both unwrap identically, and both getattr(g, "value", g) workarounds the issue cites are removed.

Two things this surfaced

A silent performance regression, nearly shipped. _contract's bare-array route sends any QuantityMatrix diagonal to the dense einsum — right for a unitful one, but the intrinsic sphere's is dimensionless, so every sphere chart would have quietly dropped the O(n) diagonal path (#686) for O(n²) while still returning correct numbers. Only the bare-in/bare-out type error exposed it. The dimensionless case is now unwrapped and keeps the fast path; unitful still goes dense.

The bare-array contract runs deeper than #628 suggests. norm's bare-array overload broke too: array_norm has no (QuantityMatrix, Array) overload, so an -> Array function tried to return a Quantity.

Honest scope: the container is still not uniform

cart3d       -> ArrayImpl        (bare)
minkowskict  -> ArrayImpl        (bare)
sph3d        -> QuantityMatrix
sph2 (S2)    -> QuantityMatrix   (this PR)

The split has moved from intrinsic-vs-embedded to Cartesian-vs-curvilinear. This PR removes two workarounds and test_jit, which is parametrized across both families, still needs one. So this is not "one accessor for generic consumers" library-wide; making flat metrics matrix-valued too would be a much larger change.

`QuantityMatrix` needs upstream work — filed and fixed

Every unwrap in this PR is hand-rolled because QuantityMatrix has no working conversion API:

call	result
`u.uconvert(UnitsMatrix, qm)`	✅ registered
`u.ustrip(UnitsMatrix, qm)`	❌ `NotFoundLookupError`
`qm.ustrip("")`	❌ `AttributeError: 'UnitsMatrix' object has no attribute 'to'`
`u.unit_of(qm)`	❌ `TypeError`
`jnp.allclose(qm, arr)` / `qnp.allclose(qm, arr)`	❌ no overload / same `AttributeError`

unxts.linalg registered uconvert but not ustrip, so everything needing a conversion falls through to the scalar-unit astropy path (unxt/_interop/unxt_interop_astropy/quantity.py:303, which calls x.unit.to(...)).

Filed as GalacticDynamics/unxt#879, fixed in GalacticDynamics/unxt#880.

Once that is released, three hand-rolled unwraps here collapse to one call each:

# quadratic_form._contract — currently compares against UnitsMatrix.full(shape, "")
d = u.ustrip(AllowValue, "", mm.diagonal)

# norm's bare-array overload — currently a manual isinstance + unit check + .value
gm = u.ustrip(AllowValue, "", mm.to_dense().matrix)

# tests — currently .value, since allclose rejects a QuantityMatrix
assert qnp.allclose(g.diagonal, expected)

unit_of ships in unxt#880 too, so generic code can ask a QuantityMatrix for its units. (An earlier revision of this description claimed it needed unxt's abstract return type widened — that was wrong; plum enforces each method's own annotation. The real, narrower problem is unxt#881: under a combined pytest session plum resolves -> UnitsMatrix to a second class object, so that one test is xfailed upstream.)

Verification

Full suite: 9944 passed, 8 skipped, 1 xfailed
The O(n) diagonal fast path is preserved for dimensionless metrics; unitful still routes dense
ruff/ty clean via prek

🤖 Generated with Claude Code

codecov · 2026-08-14T12:35:57Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.55%. Comparing base (cd59977) to head (7c35ef3).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #716   +/-   ##
=======================================
  Coverage   96.54%   96.55%           
=======================================
  Files         265      265           
  Lines        8780     8787    +7     
=======================================
+ Hits         8477     8484    +7     
  Misses        303      303

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR makes the intrinsic hyperspherical metric (S² / HyperSphericalManifold) return a dimensionless QuantityMatrix container (instead of a bare JAX array) so consumers/tests can unwrap metric matrices consistently, and updates related contraction/norm code paths accordingly.

Changes:

Wrap the intrinsic hypersphere diagonal metric in a dimensionless QuantityMatrix for container consistency.
Update manifold metric tests to unwrap QuantityMatrix via .value where allclose lacks overloads.
Adjust diagonal contraction (quadratic_form._contract) and bare-array norm handling to account for dimensionless QuantityMatrix metrics.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`src/coordinax/_src/spherical/register_metric.py`	Changes intrinsic hypersphere metric diagonal to a dimensionless `QuantityMatrix`; updates doctest output.
`src/coordinax/_src/spherical/metric.py`	Updates `RoundMetric` docstring example to reflect `QuantityMatrix` output.
`src/coordinax/_src/manifolds/quadratic_form.py`	Adds special-casing so dimensionless `QuantityMatrix` diagonals can keep the O(n) contraction path for bare arrays.
`src/coordinax/_src/manifolds/norm.py`	Unwraps dimensionless dense `QuantityMatrix` metric for bare-array norm, and errors on unitful metrics with bare vectors.
`tests/unit/manifolds/test_metrics.py`	Updates assertions to use `.value` for dimensionless `QuantityMatrix` diagonals.
`tests/unit/manifolds/test_metric_pullback_consistency.py`	Normalizes dense-matrix comparisons by unwrapping both metrics via `.value`.
`tests/unit/manifolds/test_metric_matrix_dispatch.py`	Updates hypersphere diagonal checks to use `.value`; keeps cross-family normalization in JIT test.
`tests/unit/manifolds/test_metric_matrix_batch_invariant.py`	Simplifies value extraction assuming `QuantityMatrix` (but leaves an outdated comment).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…tityMatrix GalacticDynamics#628 reports that the intrinsic sphere and embedded rules disagree on units. They do not: they are different metrics. Embedded is the *induced* metric, `ds` an ambient length, so `[g] = L**2/rad**2` and it scales as `R**2`. Intrinsic is the *angular* metric on the unit sphere -- `cxm.S2` has no radius -- where `ds` is the great-circle angle, so `[g]` is dimensionless. Since `rad` carries dimension *angle*, forcing `1/rad**2` onto the intrinsic metric would be dimensionally incoherent, not a convention choice. What is inconsistent is the *container*. Two of the three intrinsic rules in this module already return a dimensionless `QuantityMatrix` -- one of them carrying the comment `angles -> angles, so g is dimensionless`. The main hypersphere rule returned a bare array. It now follows its own module. Units unchanged. `norm`'s bare-array overload unwraps the metric explicitly, since `array_norm` has no `(QuantityMatrix, Array)` overload and `QuantityMatrix.ustrip` will not take a `UnitsMatrix`; it checks the units are empty rather than peeking at `.value`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`_contract`'s bare-array diagonal route sent any `QuantityMatrix` diagonal to the dense einsum. Right for a *unitful* one, but the intrinsic sphere's is dimensionless, so the previous commit would have silently dropped the O(n) path for every sphere chart and returned a `Quantity` from bare inputs. Unwrap the dimensionless case and keep the fast path; unitful still goes dense. The two `getattr(g, "value", g)` workarounds GalacticDynamics#628 cites are now removable, and are removed: with the intrinsic metric a `QuantityMatrix` like its siblings, `test_metric_pullback_consistency` unwraps both sides identically instead of one bare and one `.value` -- which was the asymmetry the issue was about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Doctests and asserts that read the intrinsic sphere's diagonal as a bare array now read a dimensionless `QuantityMatrix`. `jnp.allclose` and `qnp.allclose` both reject a `QuantityMatrix` -- `UnitsMatrix` has no `to` -- so these unwrap with `.value`, the idiom already used elsewhere for `g.matrix.value`. `test_jit` is parametrized across families and keeps a normalising `getattr(result, "value", result)`: flat charts still return a bare diagonal while curvilinear ones return a `QuantityMatrix`, so a test spanning both must still cope with either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…metrics Copilot review caught a real defect in both dimensionless checks. A batched diagonal has `d.shape == (*batch, n)` while its units cover the component axis only, `d.unit.shape == (n,)`. Comparing against `UnitsMatrix.full(d.shape, "")` therefore never matched: - `_contract` sent every batched dimensionless metric to the dense einsum, silently losing the O(n) path this branch exists to keep - `norm`'s bare-array overload raised outright: `UnitsMatrix only supports 1D or 2D, but got ndim=3` Both now build the comparison from `unit.shape`. Batched bare-array `norm` on `sph2` returns `[1. 1.]` instead of raising. Also from review: two doctests asserted the exact `QuantityMatrix` repr, which is brittle across unxt versions -- and this PR expects upstream changes -- so they check values instead; and a test comment claiming "units unchanged" no longer described a test where both sides are now `QuantityMatrix`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…array` The oldest-dependencies job failed on `jnp.asarray(Q(2., ''))`. Newer JAX coerces a Quantity to a bare array; the oldest supported version raises. The old version is the one that is right. `jnp.asarray(u.Q(2.0, 'm'))` returns `Array(2.)` -- metres silently gone -- so relying on that coercion would let a unit error pass as a number. `u.ustrip('', x)` converts to dimensionless and raises `UnitConversionError` on anything else, so the assertion now fails loudly where it used to succeed quietly. The two paths genuinely differ in type, which is why the coercion was there: the fast path unwraps a dimensionless diagonal and returns a bare array, the dense path returns a dimensionless Quantity. That is documented on the test now rather than papered over. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

… path users cannot take codecov flagged the two lines of this guard as the patch's only misses. They are unreachable through any public route: a bare `at` yields a dimensionless metric, and the unitful metrics that exist -- an embedded sphere's pullback, m2 -- belong to charts `check_metric_is_charts` rejects further up. The two ways to make codecov green were a test that bypasses the public API to reach the overload directly, pinning a path no caller can take, or saying plainly that the branch is defensive. This is the second. Not deleted: silently dropping the metric's unit here would return a norm in the wrong dimension and look perfectly fine, and a chart carrying units at a bare base point is a supportable thing to add later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (2)

tests/unit/manifolds/test_metric_matrix_batch_invariant.py:61

This comment is now internally inconsistent with the code below and with the updated S¹/S² metric rules: CURVILINEAR now yields a QuantityMatrix diagonal for both Euclidean-curvilinear and intrinsic-sphere charts (dimensionless for the spheres), so it no longer returns a bare array here. Please update/remove this comment to match the current container contract.

    # The Euclidean rules return a united QuantityMatrix; the intrinsic sphere
    # rules return a bare (dimensionless) Array. Compare whichever is carried.

src/coordinax/_src/spherical/register_metric.py:87

The doctest was updated to use g.diagonal.value here, but later in the same docstring the example still does float(g.diagonal[1]). Now that g.diagonal is a QuantityMatrix, indexing likely returns a scalar Quantity and float(quantity) is not a supported conversion pattern in this codebase (most tests use .value or u.ustrip). This will make the doctest fail or become dependency-version sensitive; unwrap via .value consistently.

    >>> bool(jnp.allclose(g.diagonal.value, jnp.array([1.0, 1.0])))
    True

The oldest-dependencies job failed all seven `TestChordDistance` cases with TypeError: Unexpected input type for array: Quantity `chord_distance` returns a dimensionless `Quantity`, and the tests fed it to raw `jax.numpy.asarray`, which on jax 0.7.2 raises. Rebasing onto GalacticDynamics#725 narrows why that spelling was wrong. unxt 2.0.2 is the floor now, and its `Quantity.__array__` raises `UnitConversionError` for anything dimensionful rather than returning a bare array -- so `jnp.asarray` is no longer the silent stripper the first version of this commit described, and neither is `float`. The conclusion survives the correction: on the oldest supported jax it still raises for *every* `Quantity`, dimensionless included, which is the failure the `check_oldest` job reported. Same reasoning GalacticDynamics#716 records for the strips it touched. `.ustrip("")` at the five call sites, naming the unit expected rather than relying on a conversion that reads as incidental. One `jnp.asarray` wrapped around an already-stripped `ustrip("rad")` goes too -- it was the last of the pattern left in the class, and it converted nothing. The doctests keep `float(...)`, which is now load-bearing rather than lax: it raises if the unit-sphere chord ever stops being dimensionless, and the embedded case prints `Distance(4., 'm')` with the unit intact. This should also clear `codecov/project`: that job uploads the coverage report, so its failure withheld it. `chord_distance.py` is at 100% and `codecov/patch` passed throughout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`--ignore-glob=docs/spec.md` carried the comment "exclude the spec file" and no reason. It was hiding 17 stale examples out of 274. GalacticDynamics#716 hit one of them directly -- it had to note that spec.md still printed the sphere diagonal as a bare `Array`, "which nothing caught because pytest carries `--ignore-glob=docs/spec.md`". Removing the glob is the fix for the next one. Most of the 17 are drift the exclusion let accumulate: charts now repr with their manifold (`Spherical3D()` -> `Spherical3D(M=Rn(3))`), `default_chart` is a method rather than a property so four examples printed `<bound method ...>`, `weak_type` moved in two directions, and the metric-level `norm` grew its required `at=` when GalacticDynamics#715 made metrics point-evaluated. One ```python block was illustrative pseudo-code -- `manifold`, `point`, `chart` are undefined -- and is now an unlabelled fence so sybil leaves it alone. Three were not cosmetic, and are called out rather than papered over: 1. `no_manifold.ndim` is `0`, not the `-1` the spec asserted. Recorded as `0` with the caveat that it therefore does *not* distinguish the sentinel from a genuine zero-dimensional manifold -- `Rn(0).ndim` is `0` too -- so callers must test `isinstance(M, NoManifold)`. Whether the sentinel *should* be distinguishable is a separate question; the source doctest pins `0` and is already under CI, so this commit follows it rather than contradicting it. 2. `guess_manifold({"theta": ..., "phi": ...})` returns `NoManifold()`, where the spec claimed `HyperSphericalManifold(ndim=2)`. Component names alone do not infer a sphere, though `x/y/z` do infer `Rn(3)` and the chart overload resolves correctly. Recorded, with a line saying to hand over the chart. 3. `TwoSphereIn3D` defaults its ambient to `Spherical3D`, so `pt_embed` returns `(r, theta, phi)`, not the `(x, y, z)` the example showed. Documented the default and how to ask for Cartesian. Passing `ambient=cxc.cart3d` was tried first and rejected: it routes through `cos(pi/2)` and puts `1.2246468e-16` into the expected output, which is precisely the brittleness this change exists to stop. (1) and (2) are behaviour the spec asserted and the code does not have. This commit makes the spec describe what is true so the file can run; if the original claims were the intent, they are two bugs rather than two doc edits. before: 17 failed, 257 passed (not run in CI) after: 274 passed (run in CI) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…purpose (#764) metric_matrix intentionally returns a bare array for flat charts and a QuantityMatrix for curvilinear and intrinsic charts. Flat charts keep the bare-array representation because their metric is the dimensionless identity and boxing adds no information while imposing a measurable cost on the pure-JAX fast path. Curvilinear charts remain boxed because their metrics carry units and scale information that a bare array cannot represent. Intrinsic charts follow the curvilinear convention because they share its consumers. Document the rationale and performance measurements in docs/api/manifolds.md, and pin the convention with test_metric_container_convention. Correct two stale comments describing pre-#716 behaviour. Also include cartnd in the flat-chart regression coverage and compare units by equality with u.unit("") rather than their string representation. Co-Authored-By: Claude Opus 5 noreply@anthropic.com⁠

github-actions Bot added 🐛 Fix a bug Fix a bug. ✅ Add / update / pass tests Add, update, or pass tests. labels Aug 14, 2026

nstarman added this to the v0.24.0 milestone Aug 14, 2026

This was referenced Aug 14, 2026

unxts.linalg: QuantityMatrix registers uconvert but not ustrip or unit_of GalacticDynamics/unxt#879

Closed

✨ feat(linalg): register ustrip for QuantityMatrix GalacticDynamics/unxt#880

Merged

nstarman marked this pull request as ready for review August 14, 2026 13:26

Copilot AI lite review requested due to automatic review settings August 14, 2026 13:26

Copilot started reviewing on behalf of nstarman August 14, 2026 13:27 View session

nstarman force-pushed the claude/intrinsic-metric-container branch from 71601bf to 0952ab1 Compare August 14, 2026 13:27

Copilot AI reviewed Aug 14, 2026

View reviewed changes

nstarman force-pushed the claude/intrinsic-metric-container branch from e99964d to 2680b1a Compare August 14, 2026 18:07

nstarman mentioned this pull request Aug 15, 2026

unxt 2.0.1 makes ty report phantom missing-argument errors in downstream dataclass constructors GalacticDynamics/unxt#889

Closed

nstarman and others added 5 commits August 15, 2026 10:55

nstarman force-pushed the claude/intrinsic-metric-container branch from 2680b1a to 49969a3 Compare August 15, 2026 09:04

github-actions Bot added the 🩹 Simple fix (non-critical) Simple fix for a non-critical issue. label Aug 16, 2026

nstarman mentioned this pull request Aug 17, 2026

⬆️ deps: adopt unxt 2.0.2, and fix the latent defects it reveals #725

Merged

nstarman requested a lite review from Copilot August 17, 2026 20:52

Copilot started reviewing on behalf of nstarman August 17, 2026 20:53 View session

Copilot AI reviewed Aug 17, 2026

View reviewed changes

nstarman merged commit 19bc065 into GalacticDynamics:main Aug 17, 2026
18 checks passed

nstarman deleted the claude/intrinsic-metric-container branch August 17, 2026 21:02

nstarman mentioned this pull request Aug 18, 2026

📝 docs(spec): specify chord_distance and interval #732

Merged

nstarman mentioned this pull request Aug 18, 2026

✅ test(spec): put docs/spec.md examples under CI #733

Merged

nstarman mentioned this pull request Aug 19, 2026

📝 docs(manifolds): the metric container is split by chart family, on purpose #764

Merged

Navigation Menu

Uh oh!

🐛 fix(spherical): return the intrinsic metric as a dimensionless QuantityMatrix - #716

🐛 fix(spherical): return the intrinsic metric as a dimensionless QuantityMatrix#716
nstarman merged 6 commits into
GalacticDynamics:mainfrom
nstarman:claude/intrinsic-metric-container

nstarman commented Aug 14, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Aug 14, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

nstarman commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The two families are not the same geometric object

What was actually inconsistent: the container

Two things this surfaced

Honest scope: the container is still not uniform

QuantityMatrix needs upstream work — filed and fixed

Verification

Uh oh!

codecov Bot commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nstarman commented Aug 14, 2026 •

edited

Loading

`QuantityMatrix` needs upstream work — filed and fixed

codecov Bot commented Aug 14, 2026 •

edited

Loading