🐛 fix(quantity): stop _repr_latex_ corrupting units without _repr_latex_ - #870
Merged
nstarman merged 3 commits intoAug 13, 2026
Merged
Conversation
…latex_`
`_repr_latex_` sliced `[1:-1]` off the unit's representation, assuming
astropy's `$...$` wrapping. A unit type with no `_repr_latex_` (e.g.
`unxts.linalg.UnitsMatrix`) falls back to plain `__repr__()`, which carries
no such wrapping -- the slice ate its first and last characters instead:
before: '$[1.,~2.,~3.] \; nitsMatrix("(m, s, kg)"$'
after: '$[1.,~2.,~3.] \; UnitsMatrix("(m, s, kg)")$'
Only strip the `$...$` wrapping when it's actually there, i.e. when
`_repr_latex_` exists; the plain `__repr__()` fallback is used unsliced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
🧩 unxts-linalg
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a LaTeX representation bug in Quantity._repr_latex_() where unit rendering was being sliced incorrectly when the unit falls back to plain __repr__() (not wrapped in $...$), corrupting the displayed unit string.
Changes:
- Update
Quantity._repr_latex_()to avoid slicing the fallback unit__repr__()output. - Add a regression test in
unxts.linalgcovering theUnitsMatrixfallback path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/unxt/_src/quantity/mixins.py | Adjusts LaTeX unit repr handling to avoid corrupting units without _repr_latex_. |
| packages/unxts.linalg/tests/test_printing.py | Adds regression test ensuring _repr_latex_() does not eat characters for UnitsMatrix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Fixes the `Format` job, which runs pylint:
src/unxt/_src/quantity/mixins.py:189:17: C2801: Unnecessarily calls
dunder method __repr__. Use repr built-in function. (unnecessary-dunder-call)
The fallback branch reached for `self.unit.__repr__()` to mirror the
`_repr_latex_` call above it. `repr(self.unit)` is the same call through the
built-in, and it fits on one line now that the conditional is shorter.
Behaviour is unchanged; the comment above still explains why this branch must
not slice its result.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
🎨 Improve code structure / format
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #870 +/- ##
==========================================
+ Coverage 99.84% 99.97% +0.12%
==========================================
Files 84 55 -29
Lines 3975 3338 -637
Branches 309 240 -69
==========================================
- Hits 3969 3337 -632
+ Misses 3 0 -3
+ Partials 3 1 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Addresses Copilot's review: the previous version keyed the strip off the
*existence* of `_repr_latex_` rather than off the output being wrapped. A unit
whose `_repr_latex_` returns an unwrapped string would still be sliced --
r"\mathrm{m}" -> "mathrm{m"
which is the same corruption this PR is for, one step removed. Check for the
delimiters instead.
The length guard matters: a lone `"$"` satisfies both `startswith` and
`endswith`, and would otherwise be sliced to nothing.
Four cases now pinned, exercising the mixin directly since no real unit type
takes the unwrapped branch: wrapped LaTeX is unwrapped exactly once, unwrapped
LaTeX is left intact, a unit without `_repr_latex_` falls back to `repr`, and a
lone `$` survives.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
✅ Add / update / pass tests
nstarman
added a commit
to nstarman/unxt
that referenced
this pull request
Aug 13, 2026
Carries across the hardening from GalacticDynamics#870, which this PR supersedes. `pparts(AbstractUnit)` sliced `to_string("latex")[1:-1]` unconditionally. That is safe for astropy units, which always wrap -- but it is the same shape as the bug GalacticDynamics#870 fixed, and it would corrupt any fragment arriving unwrapped: `\mathrm{m}` becomes `mathrm{m`. Since this engine is the extension point that downstream packages register into, the assumption is worth removing before someone else's unit type inherits it. `unwrap_math` lives in the engine rather than at the call site: it is generic markup handling with no unit knowledge, and it is the kind of rule a second markup would otherwise re-derive. The length guard is load-bearing -- a lone `"$"` satisfies both `startswith` and `endswith` and would otherwise be sliced away entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nstarman
added a commit
to nstarman/unxt
that referenced
this pull request
Aug 13, 2026
…ackport The backport was cut from GalacticDynamics#870's first commit rather than its merged state, so it missed the two changes that came out of review: - `self.unit.__repr__()` -> `repr(self.unit)`. This is what fails the `Format` job here, which runs pylint: `C2801 unnecessary-dunder-call`. - Strip `$...$` only when they are actually present, rather than keying off whether the unit implements `_repr_latex_` at all. That proxy reproduces this very defect one step removed, mangling `r"\mathrm{m}"` into `"mathrm{m"` for an implementation returning an unwrapped string. The length guard matters too: a lone `"$"` satisfies both `startswith` and `endswith`. `mixins.py` is now byte-identical to `main`. The four accompanying tests are lifted from `main`, but *only* those: `main`'s copy of this file also carries `test_pdoc_chains_caller_custom_hook` from GalacticDynamics#869, whose `_chain_custom` does not exist on this branch, so taking the file wholesale would have tested absent code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nstarman
added a commit
that referenced
this pull request
Aug 13, 2026
…epr_latex_` corrupting units without `_repr_latex_`) (#872) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
nstarman
added a commit
to nstarman/unxt
that referenced
this pull request
Aug 13, 2026
Carries across the hardening from GalacticDynamics#870, which this PR supersedes. `pparts(AbstractUnit)` sliced `to_string("latex")[1:-1]` unconditionally. That is safe for astropy units, which always wrap -- but it is the same shape as the bug GalacticDynamics#870 fixed, and it would corrupt any fragment arriving unwrapped: `\mathrm{m}` becomes `mathrm{m`. Since this engine is the extension point that downstream packages register into, the assumption is worth removing before someone else's unit type inherits it. `unwrap_math` lives in the engine rather than at the call site: it is generic markup handling with no unit knowledge, and it is the kind of rule a second markup would otherwise re-derive. The length guard is load-bearing -- a lone `"$"` satisfies both `startswith` and `endswith` and would otherwise be sliced away entirely. 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.
Split out of #855's "bugs fixed in passing" so it can land and backport independently, ahead of that PR rebasing on top.
_repr_latex_sliced[1:-1]off the unit's representation, assuming astropy's$...$wrapping. A unit type with no_repr_latex_(e.g.unxts.linalg.UnitsMatrix) falls back to plain__repr__(), which carries no such wrapping — the slice ate its first and last characters instead of just unwrapping$...$:Only strip the
$...$wrapping when it's actually there, i.e. when_repr_latex_exists; the plain__repr__()fallback is used unsliced. Nothing covered this before; a regression test is included inunxts.linalg(the concrete type that hits the fallback path) andunxt's own astropy-backed path is unaffected.