💥 boom(unitsystems): make unit-system identity order-independent#778
Merged
nstarman merged 6 commits intoJul 23, 2026
Merged
Conversation
Unit systems are now set-like: the same base units in any argument order
give the same (equal, same-type) system. Two changes in the `unitsystem`
factory:
- **Set-based construction.** The dimension *set* — not the argument
order — identifies a system. A registered class (a built-in like
`LTMAUnitSystem`, or a previously-created dynamic one) is matched by its
dimension set and built in that class's own field order, so the
built-ins keep their conventional order (galactic stays
`unitsystem(kpc, Myr, solMass, rad)`). A brand-new dimension set is
created with its units sorted by dimension name, so the field order —
and hence the class identity — is deterministic and order-independent.
- **A sequence builds from units.** `unitsystem(["km"])` now builds a
system from the units rather than unpacking to `unitsystem("km")` and
raising a named-lookup error. A bare string is still a named lookup.
To keep the natural-system realizations (planck, atomic) in their
conventional length/mass/time/… order rather than sorting, their two
shapes are pre-registered in builtin.py alongside the other built-ins.
Doctests/reprs for genuinely novel dynamic systems are updated to the new
sorted field order; built-in and named systems are unchanged.
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 PR makes unxt.unitsystems.unitsystem identity order-independent by treating unit-system construction as set-like over base dimensions, preventing permutation-based class proliferation and enabling unitsystem(["km"]) to build a length system.
Changes:
- Updated unit-system construction to match registered unit-system classes by dimension set (order-independent) and to create novel systems with a deterministic (dimension-name-sorted) field order.
- Adjusted the
Sequenceoverload so sequences are interpreted as units (fixingunitsystem(["km"])). - Pre-registered conventional “natural shape” unit-system classes so user construction preserves expected ordering for
planck/atomicshapes; updated tests and doctests accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_unitsystems.py | Adds regression and identity/order-independence tests (including natural-shape ordering). |
| src/unxt/_src/unitsystems/core.py | Implements set-based registry matching + deterministic ordering for novel dynamic systems; updates doctests. |
| src/unxt/_src/unitsystems/builtin.py | Adds pre-registered conventional-shape classes for Planck/atomic-like systems. |
| src/unxt/_src/unitsystems/base.py | Updates doctest output reflecting deterministic field order for dynamically-created systems. |
| README.md | Updates doctest output for dynamical unit-system repr ordering. |
| docs/guides/units_and_systems.md | Updates guide examples to match new deterministic ordering/representation. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #778 +/- ##
==========================================
+ Coverage 91.73% 96.56% +4.83%
==========================================
Files 82 43 -39
Lines 3689 2502 -1187
Branches 304 150 -154
==========================================
- Hits 3384 2416 -968
+ Misses 228 54 -174
+ Partials 77 32 -45 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Address PR GalacticDynamics#778 review comments: - Index registered unit-system classes by their frozenset of base dimensions (`_UNITSYSTEMS_BY_DIMSET`), populated alongside `_UNITSYSTEMS_REGISTRY` in `__init_subclass__`. Construction now hits this index in O(1) instead of linearly scanning the registry and picking the first set-equal entry, making class selection deterministic rather than dependent on registry iteration order. - Reword the `Sequence` overload summary line ("Build a unit system from a sequence of units") so it no longer implies non-unit arguments are forwarded. - Isolate the by-set index in the test fixture and clean it up alongside the registry in `test_unitsystem_already_registered`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Harden the by-dimension-set index from 3b751e7: registration now rejects a second unit-system class that spans the same dimension set in a different field order, instead of silently letting the last registration win. This fully closes the reviewer's "insertion-order dependence" concern -- the by-set index is now provably unambiguous, not merely unambiguous by accident of the current built-ins. - Validate both registries before mutating either, so a rejected registration leaves no partial entry (the exact-duplicate and set-collision checks now precede the commits to `_UNITSYSTEMS_REGISTRY` / `_UNITSYSTEMS_BY_DIMSET`). - The `slots=True` rebuild re-registers the same class (same ordered dims) and is still allowed through to overwrite; only a genuinely different ordering is a conflict. - Add a regression test covering the rejection and the no-partial-entry guarantee. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`core` imported `_UNITSYSTEMS_BY_DIMSET` by binding (`from .base import ...`), so a test that rebinds `base._UNITSYSTEMS_BY_DIMSET` for isolation left construction reading the *original* index while registration wrote to the isolated one -- lookup and registration could diverge, a latent flakiness trap (Copilot review on tests/unit/test_unitsystems.py). Read the index through the `base` module at the lookup site so registration (which mutates `base._UNITSYSTEMS_BY_DIMSET`) and lookup always resolve the same object. This mirrors how `_UNITSYSTEMS_REGISTRY` already works -- only the `base` attribute needs patching -- rather than adding a second patch point in the fixture. Add a regression test that registers a class under the isolating fixture and confirms `unitsystem(...)` returns it, which only holds when lookup and registration share the isolated index. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/unit/test_unitsystems.py:316
- Cleanup uses
_UNITSYSTEMS_REGISTRY/_UNITSYSTEMS_BY_DIMSETimported at module import time. If a test (now or later) monkeypatchesunxt._src.unitsystems.base._UNITSYSTEMS_REGISTRY/_UNITSYSTEMS_BY_DIMSET, these imported names won’t follow the patched dicts, so cleanup could mutate the wrong registry. Use the live dicts viaus_base(consistent with the regression test below).
# Clean up custom unit system from the registry and its by-set index:
del _UNITSYSTEMS_REGISTRY[MyUnitSystem._base_dimensions]
del _UNITSYSTEMS_BY_DIMSET[frozenset(MyUnitSystem._base_dimensions)]
Address PR GalacticDynamics#778 review: drop the direct `from ...base import _UNITSYSTEMS_REGISTRY, _UNITSYSTEMS_BY_DIMSET` bindings and access both dicts through `us_base.*` everywhere. Import-time bindings capture the original dict objects and silently bypass fixture monkeypatching; routing all access through the module attribute keeps registry reads and mutations consistent with the fixture-aware access used by the other tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… names Address two PR GalacticDynamics#778 review comments: - base.py: the duplicate-registration guard exempted *any* class with `__slots__` in its `__dict__` as a proxy for "the dataclass slots rebuild", so a distinct class that merely declared `__slots__` in its body silently overwrote an existing same-dimensions registration (and its by-set index entry). Replace the proxy with `_is_dataclass_slots_rebuild`, which admits only the genuine rebuild: the slotless -> slotted transition plus the annotation object shared by identity (the rebuild copies `cls.__dict__` wholesale), which no independently-defined class reproduces. Version-safe via `__annotations__` (<=3.13) / `__annotate_func__` (3.14+). - utils.py: `parse_dimlike_name(AbstractDimension)` used `zeroth(_physical_type)`, which picks an arbitrary element of an unordered physical-type list -- non-deterministic across runs / PYTHONHASHSEED, so a dynamically-created system's field order and class identity could vary. Pick `min(_physical_type)` (alphabetically-first, stable), which also yields "speed" over "velocity", retiring the ad-hoc speed special-case and the `zeroth` dependency. Regression tests added for both; full unit suite + all doctests pass. Co-Authored-By: Claude Opus 4.8 <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.
Summary
Closes an API-freeze item from the v2.0 release-gate audit: unit-system identity was order-dependent —
unitsystem("m", "s")andunitsystem("s", "m")were distinct, unequal, dynamically-generated classes, and every argument permutation permanently registered a new class. Also,unitsystem(["km"])raised instead of building a system.Unit systems are now set-like: the same base units in any order give the same (equal, same-type) system.
Changes
core.py). The set of dimensions identifies a system, not the argument order. A registered class — a built-in likeLTMAUnitSystemor a previously-created dynamic one — is matched by its dimension set and built in that class's own field order. So the built-ins keep their conventional order (galacticstaysunitsystem(kpc, Myr, solMass, rad)regardless of input order). A brand-new dimension set is created with its units sorted by dimension name, so the field order — and hence the class identity — is deterministic and order-independent.core.py).unitsystem(["km"])now builds a length system rather than unpacking tounitsystem("km")and hitting the named-system lookup. A bare string is still a named lookup (unitsystem("galactic")).builtin.py). Theplanckandatomicrealizations' two shapes (length·mass·time·temperature, length·mass·time·charge) are pre-registered so they keep conventional order instead of sorting alphabetically — matching howgalactic/si/cgsare handled.Behavior change
Only novel dynamically-created unit systems change: their field order (and therefore class name / repr) is now the deterministic sorted order. Built-in and named systems (
galactic,si,cgs,planck,atomic, …) are unchanged. Doctests for the novel cases are updated accordingly.Verification
TDD (tests written first, watched fail).
tests/unit/test_unitsystems.py41 passed; fulltests/unit/354 passed (beartype active); 1084 doctests acrosssrc/,docs/,README.mdpassed; gala-interop 30 passed. ruff + prettier clean.🤖 Generated with Claude Code