📝 docs(spec): Shear matches the code, and Linear gets a section - #762
Merged
nstarman merged 2 commits intoAug 19, 2026
Merged
Conversation
Third and fourth stale entries in the transforms spec, after the `Scale` one a reviewer caught on GalacticDynamics#750. Found by looking this time rather than by accident. `Shear` was documented with `factor : float` and `chart : AbstractChart`, neither of which it has -- `dataclasses.fields(Shear(H))` is `("H",)`. Its inverse (`Shear(-factor, chart)`) and composition (`Shear(k1) + Shear(k2)`) described an API that never existed: there is no composition operator, and piping two through `simplify` yields a `Linear`. `Linear` had no section at all. It reached the exported-objects table in GalacticDynamics#730 but was never specified, which is why GalacticDynamics#750 had to drop a `Scale` -> `Linear` link rather than point at a missing anchor. The anchor exists now, so that link is restored. Every claim checked against the running code: fields, `inverse`, what `simplify` returns, and that a fused rotation-and-reflection reports `OrthogonalGroup`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the transforms portion of docs/spec.md to reflect the current coordinax.transforms implementation, correcting the documented Shear API/behavior and adding a missing spec section for Linear so that doc anchors and references are valid.
Changes:
- Fixes the
Shearspec to document its actual single field (H) and itssimplify/fusion behavior. - Adds a new
Linearspec section (math definition, fieldsM/staticgroup, inverse behavior, and fusion semantics). - Restores/updates the
Scale→Linearcross-reference using the newLinearanchor.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
github-actions
Bot
added
the
📝 Add / update documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #762 +/- ##
=======================================
Coverage 96.63% 96.63%
=======================================
Files 267 267
Lines 9070 9070
=======================================
Hits 8765 8765
Misses 305 305 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review follow-up: I wrote that inverting a shear "negates the off-diagonal
entries", generalising from the one elementary shear I checked. It is false as
soon as two off-diagonals chain, because the nilpotent part has a non-zero
square:
H = [[1, 2, 0], [0, 1, 3], [0, 0, 1]]
inv(H) = [[1, -2, 6], [0, 1, -3], [0, 0, 1]]
^^ the N^2 term, not -0
`Shear` stores an arbitrary square matrix and inverts it with `jnp.linalg.inv`,
so the spec now says just that.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nstarman
added a commit
that referenced
this pull request
Aug 20, 2026
Three spec sections have now been found describing fields the class does not have -- `Scale` (#750), `Shear` (#762), and `Tangent` here -- each by accident rather than by looking. This closes the class: every `!!! info` section with a **Fields** block is checked against `dataclasses.fields`. `Tangent` was the fifth stale entry. It listed a `manifold` field that has never existed; the manifold is reached through `M`, a property derived from the chart. Its "Post-init checks" were wrong too -- the hook is `__check_init__`, and it calls `M.check_chart(chart)`, not `manifold.has_chart(chart)`. Two deliberate limits: Sets, not sequences. `Translate` defines its own `__init__` whose parameter order is the useful one to document and differs from `dataclasses.fields`; enforcing order would fail a section that is right. Per-type sections only. The exported-objects table is a curated summary -- the `coordinax.charts` row omits 50-odd class names on purpose -- so it is not an `__all__` mirror and is left alone. Both spellings of a Fields block are parsed, bullet list and table, and a guard test asserts the parser still finds sections: a regex that quietly matches nothing would pass forever. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
The third and fourth stale entries in the transforms spec, after the
Scaleone areviewer caught on #750. Found by looking this time, rather than by tripping over them.
Sheardocumented an API it never haddataclasses.fields(Shear(H))is("H",). There is nofactorand nochart— thesame fiction the
Scalesection carried. Its inverse and composition were wrong too:shear.inverse == Shear(-factor, chart)Shear(inv(H))Shear(k1) + Shear(k2) == Shear(k1 + k2)simplify(a | b)yields aLinearLinearhad no sectionIt reached the exported-objects table in #730 but was never specified. That is why #750
had to delete a
Scale→Linearlink rather than ship a dangling anchor. The sectionnow exists — maths, both fields (
Mand the staticgroup), inverse, and howsimplifysets the fused
group— and that link is restored.Verification
Every claim was checked against the running code rather than copied from a docstring:
nox -s docs(the Docs (No Warnings) job) builds clean, so the new anchor resolves —worth stating explicitly, since a dead anchor is exactly what I avoided in #750.
Spec examples are under CI as of #733: 1369 passed.
Note for later
This is the third section found stale by accident. A test asserting each
!!! infosection's Fields against
dataclasses.fieldswould close the class properly. I didnot add one here because it is a different kind of change from a docs correction, and
because the scope needs care — the exported-objects table is deliberately curated (the
chartsrow omits 50+ names on purpose), so only the per-type sections can be enforcedthat way. Happy to follow up if wanted.