✨ feat(charts): correct & complete PoincarePolar6D (symplectic phase-space chart + gala transform)#587
Open
nstarman wants to merge 3 commits into
Open
Conversation
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>
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #587 +/- ##
==========================================
- Coverage 94.66% 94.66% -0.01%
==========================================
Files 247 247
Lines 7894 7925 +31
==========================================
+ Hits 7473 7502 +29
- Misses 421 423 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…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>
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.
Comment on lines
+1778
to
+1781
| del usys | ||
| if len(from_chart.factors) != 2 or not all( | ||
| isinstance(f, Cart3D) for f in from_chart.factors | ||
| ): |
Comment on lines
+1793
to
+1804
| rho = jnp.hypot(x, y) | ||
| phi = jnp.atan2(x, y) # gala convention: azimuth from +y | ||
| lz = x * vy - y * vx | ||
| s = jnp.sqrt(2 * jnp.abs(lz)) | ||
| return { | ||
| "rho": rho, | ||
| "pp_phi": s * jnp.cos(phi), | ||
| "z": z, | ||
| "dt_rho": (x * vx + y * vy) / rho, | ||
| "pp_phidot": s * jnp.sin(phi), | ||
| "dt_z": vz, | ||
| } |
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>
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.
Comment on lines
+1749
to
+1750
| Forward only: ``sqrt(|Lz|)`` discards ``sign(Lz)``, so there is no inverse. | ||
|
|
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