✨ feat(charts): correct & complete PoincarePolar6D (symplectic phase-space chart + gala transform) - #587
Open
nstarman wants to merge 9 commits into
Open
Conversation
github-actions
Bot
added
📝 Add / update documentation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request corrects the PoincarePolar6D predefined 6D chart so its 5th coordinate matches the intended symplectic quadrature partner used by gala’s Poincaré variables (rather than a misinterpreted time-derivative placeholder), aligning component naming, units, tests, and the authoritative spec.
Changes:
- Renamed the 5th component from
dt_pp_phitopp_phidotand updated its dimension tolength / time**0.5. - Updated
PoincarePolar6Ddocstring to describe the symplectic-polar phase-space interpretation and the deliberateno_manifoldchoice (no metric / nometric_matrix). - Updated the predefined-chart unit test and
docs/spec.mdto match the corrected components/dimensions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/unit/charts/test_predef_charts.py |
Updates the expected component name and dimension for the predefined poincarepolar6d chart. |
src/coordinax/_src/charts/d6.py |
Renames the chart component key, corrects its dimension, and revises documentation/comments for the symplectic phase-space meaning. |
docs/spec.md |
Updates the authoritative spec’s component list and coordinate dimensions for PoincarePolar6D. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #587 +/- ##
==========================================
+ Coverage 94.75% 94.83% +0.07%
==========================================
Files 248 248
Lines 7958 7999 +41
==========================================
+ Hits 7541 7586 +45
+ Misses 417 413 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nstarman
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
docs/spec.md:1692
- The spec still says the only in-core transition behavior is the identity transform, but this PR also registers a new forward
pt_mapfrom Cartesian phase space (cart3d × cart3dproduct) toPoincarePolar6D. The spec should be updated so it remains the authoritative source of truth for supported transitions.
- `poincarepolar6d` is the pre-defined `PoincarePolar6D()` instance.
- Transition behavior currently registered in-core:
identity transform only (`pt_map(p, PoincarePolar6D, PoincarePolar6D) -> p`).
- No dedicated Cartesian projection dispatch is currently defined for this chart family.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
src/coordinax/_src/charts/register_ptmap.py:1782
- Add the standard manifold/chart consistency assertions (as used by other pt_map dispatches in this module). Without these, a caller could pass a manifold instance that doesn’t match the charts and still get a result.
del usys
if len(from_chart.factors) != 2 or not all(
isinstance(f, Cart3D) for f in from_chart.factors
):
msg = (
src/coordinax/_src/charts/register_ptmap.py:1851
- Same as above: add the standard manifold/chart consistency assertions so this dispatch rejects mismatched manifold+chart combinations early and consistently.
del usys
if len(to_chart.factors) != 2 or not all(
isinstance(f, Cart3D) for f in to_chart.factors
):
msg = (
docs/spec.md:1692
docs/spec.mdstill claims the only registered transition forPoincarePolar6Dis the identity transform, but this PR registerspt_maptransitions to/from a Cartesian phase-space product chart. Update the spec text so the authoritative spec matches the implemented behavior.
- Transition behavior currently registered in-core:
identity transform only (`pt_map(p, PoincarePolar6D, PoincarePolar6D) -> p`).
- No dedicated Cartesian projection dispatch is currently defined for this chart family.
PoincarePolar6D declared its 5th coordinate `dt_pp_phi` with dimension `length/time**1.5` (a literal d/dt of `pp_phi` reading). But the authoritative definition — gala's `cartesian_to_poincare_polar` (Papaphilippou & Laskar 1996, A&A 307, 427) — makes it the *symplectic quadrature partner* `sqrt(2|Lz|)*sin(phi)`, with the SAME `length/time**0.5` dimension as `pp_phi`, not its time derivative. The `time**1.5` unit and `dt_` name were a placeholder inconsistency. * rename component `dt_pp_phi` -> `pp_phidot` (gala's `p_phi_dot`) * fix its dimension `length/time**1.5` -> `length/time**0.5` * document the chart as Poincaré *symplectic-polar* variables on phase space `T*R^3`, why it has no Riemannian metric (`M = no_manifold`: phase space is symplectic, `metric_matrix` is inapplicable) and no global Cartesian chart, and the forward-only / sign(Lz)-ambiguity caveat * drop the stale `# TODO: actual manifold` markers (no_manifold is correct) Updates the predefined-chart test and spec.md accordingly. (The gala forward transform from a Cartesian phase-space source chart is a follow-up: it needs a new 6D phase-space chart, which does not exist yet.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…arePolar6D Register the pt_map from a two-factor Cartesian product chart (cart3d x cart3d = [position, velocity]) to PoincarePolar6D, implementing gala's cartesian_to_poincare_polar (Papaphilippou & Laskar 1996): rho = hypot(x, y); phi = atan2(x, y) [gala azimuth convention] Lz = x*vy - y*vx; s = sqrt(2|Lz|) pp_phi = s*cos(phi); pp_phidot = s*sin(phi) dt_rho = (x*vx + y*vy)/rho; dt_z = vz Reuses the existing product chart as the source (no new public chart). Forward only — sqrt(|Lz|) drops sign(Lz), so there is no inverse (matching gala). A non-(Cart3D, Cart3D) product source raises NotImplementedError. Known-value doctest covers the symplectic quadratures and the guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds the reverse pt_map (PoincarePolar6D -> cart3d x cart3d), derived algebraically from the forward gala map: s = hypot(pp_phi, pp_phidot); phi = atan2(pp_phidot, pp_phi); Lz = s**2/2 x = rho sin(phi); y = rho cos(phi); z = z vx = sin(phi) dt_rho - cos(phi) Lz/rho vy = cos(phi) dt_rho + sin(phi) Lz/rho; vz = dt_z Partial inverse: the forward's sqrt(2|Lz|) discards sign(Lz), so this assumes Lz >= 0 and is exact only for non-negative-Lz points (position always round-trips; velocity sign is lost otherwise). Guards a non-(Cart3D, Cart3D) target with NotImplementedError. Round-trip doctest included. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- add manifold/chart consistency asserts to both new pt_map dispatches - guard dt_rho against 0/0 on the axis (rho == 0) -> dt_rho = 0 by convention - clarify forward docstring: no global inverse, but a partial (Lz >= 0) one exists - update docs/spec.md to list the new gala forward + partial-inverse transitions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… call `CartesianProductChart((cart3d, cart3d))` omitted the required factor_names arg; describe it as "a CartesianProductChart with factors (cart3d, cart3d)". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
…elper Consolidate the "divide by a coordinate that vanishes on an axis" pattern into `_ratio_zero_on_axis`, using the double-`where` idiom so both the value and its reverse-mode gradient stay finite on the axis. A plain `jnp.where(denom == 0, 0, num / denom)` returns the right value but leaks NaN into gradients (`0 * inf`). Applied to all three sites in register_ptmap.py: - LonCosLat -> Cart3D pole (`cos(lat) == 0`) - PoincarePolar6D forward `dt_rho` (already grad-safe; unified for clarity) - PoincarePolar6D partial inverse `lz / rho` (was the plain, grad-unsafe form) Adds a regression test asserting finite value + gradient at the singularity. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lat)==0 The _ratio_zero_on_axis guard triggers only when cos(lat) is exactly 0; a floating-point near-pole value (cos(pi/2) ~= 6e-17) still divides but the cos(lat) factor multiplies the result back, keeping x, y finite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…kip sqrt - Extract `_require_cart3d_phase_space(chart, *, direction)` used by both the forward and inverse PoincarePolar6D dispatches (single source for the two-factor Cart3D check and error wording). - Compute the inverse-map `lz = (pp_phi**2 + pp_phidot**2) / 2` directly instead of `hypot(...)**2 / 2`, avoiding an unnecessary sqrt. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nstarman
force-pushed
the
claude/v024-poincarepolar6d
branch
from
July 28, 2026 04:21
a0f2fa6 to
f93b212
Compare
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.
Summary
Makes
PoincarePolar6Dcorrect and usable.1. Fix the chart declaration
The 5th coordinate
dt_pp_phiwas declaredlength/time**1.5(a literal "d/dt ofpp_phi" reading). Per gala'scartesian_to_poincare_polar(Papaphilippou & Laskar 1996) it's the symplectic quadrature partner√(2|Lz|)·sin(φ), samelength/time**0.5dimension aspp_phi. Confirmed with the maintainer that gala's symplectic(q, p)object is intended.dt_pp_phi→pp_phidot; fix its dimension tolength/time**0.5T*ℝ³; whyM = no_manifold(symplectic, no Riemannian metric) and no global Cartesian chart# TODO: actual manifoldmarkers2. Forward + partial-inverse
pt_map(both directions)Between a two-factor Cartesian product chart (
cart3d × cart3d= [position, velocity]) andPoincarePolar6D— reusing the existing product chart, no new public chart.rho=hypot(x,y),phi=atan2(x,y),Lz=x*vy−y*vx,pp_phi=√(2|Lz|)cos φ,pp_phidot=√(2|Lz|)sin φ,dt_rho=(x*vx+y*vy)/rho,dt_z=vz(vx,vy). Since√|Lz|dropssign(Lz), it assumesLz ≥ 0and is a partial inverse — position always round-trips, velocity sign is lost forLz < 0.Non-
(Cart3D, Cart3D)product sources/targets raiseNotImplementedError.Verification
Known-value + round-trip doctests (symplectic quadratures, both directions, the guard);
tests/unit/chartspasses (443); module doctests pass (298);ruff/tyclean.🤖 Generated with Claude Code