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

[nstarman]

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]

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.

[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