✨ feat(transforms): kinematic acts for Scale/Reflect/Shear without a base point by nstarman · Pull Request #584 · GalacticDynamics/coordinax

nstarman · 2026-07-25T02:16:27Z

Summary

Only Boost and Rotate had TangentGeometry (velocity/acceleration) act paths. The other linear transforms (Scale, Reflect, Shear) fell through to the generic jet prolongation, which requires a base point at — even for a constant Cartesian map where none is mathematically needed. So:

cxfm.act(cxfm.Scale.from_factors([2, 3, 4]), None, velocity, cxc.cart3d, cxr.coord_vel)
# TypeError: pushforward(Scale, ...) on order-1 tangent data requires the base point ...

while the same call on Rotate works. Surfaced by the v0.24 pre-release audit.

Fix

A linear map's Jacobian is its (constant) matrix, so a Cartesian velocity/acceleration transforms as v → M·v with no anchor. This registers a shared pushforward on AbstractLinearTransform:

Cartesian chart → apply M directly, no at needed (matches Rotate).
Non-Cartesian chart → push the tangent through the chart Jacobian (which does need at), apply M in Cartesian, pull back.
Time-dependent linear maps still route through the generic prolongation (the Ṁ term genuinely needs the jet anchors).

Rotate keeps its own, more-specific dispatch, so there is no plum ambiguity. The new path also reports mismatched tangent/anchor components with a clear TypeError instead of a raw KeyError.

Verification

Keystone property: the no-at fast path equals the generic prolongation when at is supplied.
Scale/Reflect/Shear on velocity and acceleration; non-Cartesian still requires at.
New unit tests (test_spatial_linear_transforms.py, test_prolongation.py) + a doctest; full tests/unit/transforms suite passes (265).

One pre-existing test (test_static_scale_vel_requires_at) encoded the old "requires at" behavior and is inverted to assert the new, correct behavior.

🤖 Generated with Claude Code

Copilot

Pull request overview

This PR extends kinematic (velocity/acceleration) transform support to the remaining constant linear transforms (Scale, Reflect, Shear) by adding a shared pushforward implementation on AbstractLinearTransform that applies the constant Jacobian matrix directly in Cartesian coordinates, avoiding the unnecessary requirement for a base point (at) on Cartesian charts.

Changes:

Added a shared pushforward(AbstractLinearTransform, ...) path for tangent-geometry CDicts that applies M·v without requiring at on Cartesian charts, and uses chart Jacobians (requiring at) for non-Cartesian charts.
Added/updated unit tests asserting that constant linear transforms act on Cartesian velocities/accelerations without at, and that non-Cartesian charts still require at.
Updated a previously “requires at” test to reflect the new, intended behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`src/coordinax/transforms/_src/actions/linear.py`	Adds shared tangent-geometry pushforward for constant linear transforms, enabling `act(..., coord_vel/coord_acc)` without `at` on Cartesian charts.
`tests/unit/transforms/test_spatial_linear_transforms.py`	Adds new tests covering velocity/acceleration actions for `Scale`/`Reflect`/`Shear` without `at`, plus a “fast-path equals generic-with-at” keystone check.
`tests/unit/transforms/test_prolongation.py`	Updates the static-scale velocity behavior test and adds a non-Cartesian “still requires at” assertion.

codecov · 2026-07-25T02:39:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.02%. Comparing base (780131d) to head (ff5684e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #584      +/-   ##
==========================================
+ Coverage   94.75%   95.02%   +0.26%     
==========================================
  Files         248      242       -6     
  Lines        7958     7887      -71     
==========================================
- Hits         7541     7495      -46     
+ Misses        417      392      -25

☔ 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

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

src/coordinax/transforms/_src/actions/linear.py:380

The doctest example in this docstring mixes acceleration units (m/s2) into a coord_vel call. That’s semantically inconsistent and can mislead readers (and any doctest-based docs checks). Make the example purely velocity (all m/s) or switch the representation to coord_acc and use all m/s2.

    >>> v = {"q.x": u.Q(1.0, "m/s"), "q.y": u.Q(1.0, "m/s"), "q.z": u.Q(1.0, "m/s"),
    ...      "p.x": u.Q(1.0, "m/s2"), "p.y": u.Q(1.0, "m/s2"), "p.z": u.Q(1.0, "m/s2")}
    >>> out = cxfm.act(op, None, v, ps, cxr.tangent_geom, cxr.coord_vel)
    >>> [out[k].value.round(3) for k in ("q.x", "q.y", "q.z", "p.x", "p.y", "p.z")]

Copilot

Pull request overview

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

Copilot

Pull request overview

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

Copilot

Pull request overview

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

Copilot

Pull request overview

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

Copilot

Pull request overview

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

Copilot

Pull request overview

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

…base point Only Boost and Rotate had TangentGeometry (velocity/acceleration) act paths; the other linear transforms fell through to the generic jet prolongation, which requires a base point `at` even for a constant Cartesian map where none is needed (`act(Scale, None, velocity, cart3d, coord_vel)` raised TypeError). A linear map's Jacobian is its (constant) matrix, so a Cartesian velocity or acceleration transforms as `v -> M v` with no anchor. Register a shared `pushforward` on `AbstractLinearTransform`: Cartesian charts apply `M` directly (no `at`); non-Cartesian charts push through the chart Jacobian (which does need `at`); time-dependent linear maps still route through the generic prolongation for the `dot(M)` term. Rotate keeps its own (more specific) dispatch, so there is no ambiguity. A clear TypeError now reports tangent/anchor components that do not match the chart. Verified: fast path equals the generic prolongation when `at` is supplied (the suite's keystone property); Scale/Reflect/Shear velocity + acceleration; non-Cartesian still requires `at`. New unit tests + doctest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…uct charts The new AbstractLinearTransform `pushforward` matched every chart, so on a Cartesian-product chart it applied `op._matrix(chart.cartesian, tau)` against the full product dimension and errored (e.g. a 3x3 Scale on a 6D phase-space cart3d x cart3d velocity). Add a product-chart dispatch mirroring the point action: split per factor, apply the matrix only to factors whose Cartesian dimension matches the operator, pass the rest through. Tests: factorwise velocity on cart3d x cart3d; non-Cartesian (spherical) pushforward round-trip via the `at` path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The keystone test compared `act(at=None)` vs `act(at=at)`, but for a static linear map both go through the identical pushforward path, so it validated nothing. Compare the no-`at` fast path to `cxfm.prolong` (generic autodiff) with `at` as jet slot 0, per Copilot review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The single test mixed m/s (q.*) with m/s2 (p.*) under coord_vel, which is semantically ambiguous and left the order-2 coord_acc path unexercised. Split into a pure-velocity test (all m/s, coord_vel) and a pure-acceleration test (all m/s2, coord_acc), per Copilot review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…orward Lines 295/313 used `chart is cart` while the same file uses `chart != cart` at 98/128 for the identical "is this already its Cartesian chart?" test. The `is` form works only because `.cartesian` returns `self`; `==` is consistent with the rest of the file and robust to any chart whose `.cartesian` returns an equal-but-distinct instance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…doctest The doctest mixed m/s (q.*) with m/s2 (p.*) under coord_vel, contradicting the now-split velocity/acceleration tests. Use all-m/s velocity data so the example is unit-consistent with its coord_vel semantics, per Copilot review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Two patterns were copied verbatim across the linear/prolong actions: - the "components do not match ...; missing ...; unexpected ..." TypeError appeared 3x (linear pushforward, prolong pushforward, prolong jet). Extract `require_matching_keys(actual, expected, message)` into actions/utils. - the pack-to-common-unit -> einsum -> cdict Cartesian matrix-apply appeared 3x in linear.py. Extract a local `_matmul_cdict(matrix, d, comps)` helper. Behavior and error messages are unchanged (the message-assertion tests still pass). Also drops a now-redundant cast and a stale `ty: ignore`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

`_linear_pushforward_cdict` was a private helper with two callers: the public `pushforward(AbstractChart)` dispatch (a 1-line pass-through) and the factorwise `pushforward(AbstractCartesianProductChart)` path. Every other factorwise/ composed/add action recurses through the *public* `cxfmapi.act`/`pushforward`; the private indirection was the odd one out. Inline the body into the public `pushforward` dispatch and have the factorwise path re-dispatch through `cxfmapi.pushforward` per factor — mirroring the point-action factorwise's `cxfmapi.act` (and correctly recursing on nested product factors, which the private call could not). Net -22 lines, no behavior change (full transforms suite + doctests pass). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

… charts Guards the re-dispatch in the product-chart `pushforward`: a factor that is itself a product chart routes back through the public `pushforward` and is handled recursively. A 3x3 Scale on a chart whose factors are a 6D `cart3d x cart3d`, a 3D `cart1d x cart1d x cart1d`, and a plain `Cart3D` passes the two nested factors through and scales only the Cart3D — matching the point action. The 3D nested factor is the keystone: a non-recursive direct-helper call would flatten its three 1D sub-factors into one 3-vector and scale them (2, 3, 4); recursion correctly leaves them (1, 1, 1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

- product-chart pushforward: validate v/at keys against chart.components up front (clear TypeError via require_matching_keys instead of a raw KeyError from split_components), mirroring the non-product overload - non-Cartesian 'at'-required error: reference the actual `rep` rather than a hard-coded "TangentGeometry" that the signature doesn't take - tests: strip units via `_to_np`/`u.ustrip` instead of reading `.value` directly in the product-chart factorwise/nested tests Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…rozen-τ - Detect a Cartesian/flat chart with `is_flat_chart(chart)` (isinstance-based) instead of exact `chart == cart` equality, so a flat chart that isn't the exact canonical instance still takes the at-free fast path. - Reword the docstring: this overload is the frozen-τ rule; the act-level router (`prolong`, branching on `is_time_dependent`) sends time-dependent order≥1 acts to the generic prolongation (the `dot(M)` term), so they don't reach this frozen-τ path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

src/coordinax/transforms/_src/actions/linear.py:377

In the Cartesian-product pushforward overload, _maybe unconditionally accesses factor_chart.cartesian before checking whether the factor should be acted on. This can raise NoGlobalCartesianChartError for factors without a global Cartesian chart even when the factor’s dimension doesn’t match the operator matrix and should simply pass through unchanged (per the docstring). Consider first checking factor_chart.ndim/factor_chart.components against n before touching .cartesian, so dimension-mismatched factors can short-circuit safely.

        cart = factor_chart.cartesian
        if cart.ndim != n or len(cart.components) != n:
            return part
        return cast(

Copilot AI review requested due to automatic review settings July 25, 2026 02:16

github-actions Bot added ✅ Add / update / pass tests Add, update, or pass tests. ✨ Introduce new features Introduce new features. labels Jul 25, 2026

Copilot started reviewing on behalf of nstarman July 25, 2026 02:16 View session

Copilot AI reviewed Jul 25, 2026

View reviewed changes

Comment thread src/coordinax/transforms/_src/actions/linear.py Outdated

nstarman force-pushed the claude/v024-linear-velocity branch from 0d2f2c9 to 633fae3 Compare July 25, 2026 02:25

nstarman added this to the v0.24.0 milestone Jul 25, 2026

Copilot AI review requested due to automatic review settings July 27, 2026 14:11

github-actions Bot added the 🐛 Fix a bug Fix a bug. label Jul 27, 2026

Copilot started reviewing on behalf of nstarman July 27, 2026 14:12 View session