♻️ refactor(charts): component_domains moves into core - #807
Merged
nstarman merged 3 commits intoSep 2, 2026
Conversation
A chart's legal coordinate values were declared twice: core wrote its bounds inline at the `checks.polar_range` calls, and `coordinaxs.hypothesis` kept its own `POLAR`/`LATITUDE`/`AZIMUTH` table so its strategies could generate valid points. GalacticDynamics#772 pinned the two equal with a test and left the merge as a separate decision. This is that merge. `Interval` and `component_domains` now live in `coordinax.charts`, the strategy package re-exports them, and `check_data` reads its bounds from the same declaration -- so the numbers are stated once. What core *enforces* stays exactly what it enforced: the polar and latitude intervals. `AZIMUTH` and `RADIAL` are declared and deliberately not checked -- an azimuth outside [-pi, pi] is a legal coordinate on another sheet, not an error. Declaring them anyway is what lets a generator produce points in the chart's fundamental domain. This completes the split argued in GalacticDynamics#740: dimension belongs in the container (GalacticDynamics#752 made that side authoritative), topology belongs in the domain. The domain side was the half still living outside core. `_src/constants.py` held `Deg0`/`Deg90`/`Deg180` for these checks alone and is deleted; `test_every_module_is_imported` catches it as dead otherwise. Verification: 11298 passed. `prek run --all-files` clean, `nox -s docs` succeeds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
📝 Add / update documentation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes chart component domain declarations in core (coordinax.charts) so the same single source of truth is used both by construction-time validation (check_data) and by coordinaxs.hypothesis strategies, eliminating drift between duplicated tables.
Changes:
- Introduces
Interval+component_domains(chart)in core and registers domains for built-inR^nandS^nspherical charts via dispatch. - Rewires core validation (
checks.polar_rangeand relevantcheck_dataimplementations) to read enforced bounds from the shared domain declaration and removes now-dead degree constants. - Updates/extends the agreement tests and documentation to reflect the new public API surface and the “declared vs enforced” design.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/charts/test_domain_agreement.py | Updates tests to read domains from core and adds coverage for newly included loncoslat charts plus a re-export identity check. |
| src/coordinax/charts.py | Exposes Interval and component_domains on the public coordinax.charts API. |
| src/coordinax/_src/charts/domains.py | Adds the core Interval type, canonical intervals, and the abstract component_domains dispatch surface. |
| src/coordinax/_src/charts/register_domains.py | Registers component_domains implementations for built-in R^n charts, including composite/product charts. |
| src/coordinax/_src/spherical/register_domains.py | Registers component_domains implementations for intrinsic hyperspherical charts (unit-sphere variants). |
| src/coordinax/_src/charts/init.py | Ensures domain definitions and registrations are imported so dispatch rules are active. |
| src/coordinax/_src/spherical/init.py | Ensures spherical domain registrations are imported so dispatch rules are active. |
| src/coordinax/_src/charts/checks.py | Sources polar-range endpoints from the shared core declaration instead of inline constants. |
| src/coordinax/_src/charts/d3.py | Updates latitude/polar checks to use shared endpoint constants where explicitly passed. |
| src/coordinax/_src/spherical/chart.py | Updates latitude/polar checks to use shared endpoint constants. |
| src/coordinax/_src/constants.py | Removes dead degree-angle constants now replaced by shared endpoint declarations. |
| packages/coordinaxs.hypothesis/src/coordinaxs/hypothesis/charts/_src/domains.py | Re-exports core Interval/component_domains while preserving hypothesis-level exports of common intervals. |
| packages/coordinaxs.hypothesis/src/coordinaxs/hypothesis/charts/_src/cdict.py | Adjusts typing to account for the re-exported core dispatch return type. |
| docs/spec.md | Documents component_domains/Interval semantics and the “declared vs enforced” contract. |
| docs/api/charts.md | Adds component_domains to the charts API documentation index. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #807 +/- ##
==========================================
+ Coverage 96.87% 96.90% +0.03%
==========================================
Files 270 272 +2
Lines 9374 9412 +38
==========================================
+ Hits 9081 9121 +40
+ Misses 293 291 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Three things the merge left behind. `Interval.endpoints` carried a `unit is None` guard, a `min`/`max is None` guard and an `Angle`/`Q` ternary for callers that do not exist: its only two call sites are `POLAR` and `LATITUDE`, both angular with both bounds set. One line now, and `_ANGLE` goes with it. `coordinaxs.hypothesis`'s `domains` re-exported `AZIMUTH`, `LATITUDE`, `POLAR` and `RADIAL`, which nothing imports -- only `FREE` has a consumer -- and none of them were in that module's `__all__` before, so the re-export was widening the package's public surface with unused names. The docstrings described how the code got here rather than what it does: which PR moved what, what used to be declared where, which table was pinned equal to which. Rewritten to state the design -- one declaration, read by the construction checks and the strategies both; enforcement a deliberate subset of it. The one piece of history worth keeping is now a reason instead of a date: the S^n registrations sit in `_src/spherical` because that package imports `_src/charts`, so putting them together would invert the dependency. Verification: 11658 passed, 311 skipped, 1 xfailed. `prek run --all-files` clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
🎨 Improve code structure / format
`check_data` refuses `mu < Delta^2` and `|nu| > Delta^2`, while `component_domains` fell through to the `AbstractChart` default and called every component free. Enforcement was a *superset* of the declaration, the inverse of the invariant the module states -- and the generators, which read the declaration, would have drawn points the chart rejects. `Delta` is a per-instance parameter, so this is the one domain not fixed by the chart's type. Dispatch already passes the instance, so no machinery changes; the bound is derived from the same `Delta` that `check_data` compares against. Two consequences worth naming: the domain cannot be read under `jit` (a bound is a `float`, a dynamic `Delta` is a tracer), and nothing on the construction path reads a domain, so tracing is unaffected. An audit of every `checks.*` call site confirms this was the only chart whose enforcement outran its declaration. Three edge cases fell out of wiring it to the strategies, each a pre-existing looseness that an all-`FREE` domain had been hiding: - `Delta` is constrained positive and scalar but *not* to a length, and the components are declared `area`. Seconds give a bound in `s2` that no `mu` in `m2` can be compared against; `1e200 m` squares to infinity. Neither is a bound, so neither is declared. - A bound is written in the domain's unit and drawn in another, and float32 cannot always span the gap: `mu >= 1 kpc2` is `9.5e38 m2`, past float32's `3.4e38` ceiling, and every area unit the package draws is metre-scale. The draw now falls back to the domain's own unit, which by construction can hold it. - `Delta = 32 m` floors `mu` at `1024 m2`, above the default magnitude cap of 1000, which would have asked for `1024 <= mu <= 1000`. The domain is a constraint and the cap is ergonomics, so the cap is dropped. Verification: 11667 passed, 311 skipped, 1 xfailed. `prek run --all-files` clean. The new declaration test fails without the fix. 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.
A chart's legal coordinate values were declared twice. Core wrote its bounds inline at the
checks.polar_rangecalls;coordinaxs.hypothesiskept its own table so its strategies could generate valid points:#772 pinned the two equal with a test and said so explicitly: "What this deliberately does not do: unify them... leaves the merge as a separate decision." This is that merge — and it closes the last half of the #740/#752 split that was not authoritative in core.
What moves
Intervalandcomponent_domainsnow live incoordinax.charts.coordinaxs.hypothesis.chartsre-exports them, sofrom coordinaxs.hypothesis.charts import component_domainsstill works and the strategies are unchanged.check_datareads its bounds from the same declaration, so the numbers are stated once.Split across three files, for a reason each:
_src/charts/domains.py—Intervaland the intervals. Imports no chart, sochecks.pycan readPOLAR_ENDPOINTSfrom it without a cycle._src/charts/register_domains.py— the R^n chart→interval table, kept as one table because theSpherical3D/MathSpherical3Dtheta-phi swap is the whole reason the lookup is dispatched, and that is only legible when the two sit adjacent._src/spherical/register_domains.py— the S^n charts, next to their definitions, following the existingregister_*convention rather than adding an import edge from_src/chartsinto_src/spherical.Declaring is not enforcing
This is the design point, and the reason the merge is not just a file move. What core enforces stays exactly what it enforced.
POLARandLATITUDEare checked at construction.AZIMUTHandRADIALare declared and deliberately not checked — an azimuth outside[-pi, pi]is a legal coordinate on another sheet, and a signed radius is meaningful under the antipodal map. The naive merge, having the basecheck_datawalkcomponent_domainsand enforce all of it, would newly reject both and break a great deal of working code.So the table is a superset of what is enforced, and that is on purpose: the declaration exists so a generator can produce points in the chart's fundamental domain, which is the whole reason the hypothesis package needed a table in the first place.
Interval.marginis advisory for the same reason andendpoints()ignores it —theta = 1e-30 radis mathematically legal and numerically at the pole, where the Jacobian is singular to working precision. That distinction is now written down inspec.mdrather than being implicit in which package held which number.Two things fell out
LonCosLatSpherical3DandLonCosLatSphericalTwoSphereenforce a latitude bound in core but had no entry in the hypothesis table, so they silently fell to the unconstrained default and were generating latitudes core would reject. Visible once the two tables became one; both now declarelat: LATITUDE, and both are added to the agreement test._src/constants.pyheldDeg0/Deg90/Deg180for these checks alone, and is deleted —test_every_module_is_importedcaught it as dead the moment the last import went.The test
test_domain_agreement.pykeeps the half that earns its place. The equality direction is now near-tautological, so what it pins is the other one: that a value past a declared bound is actually refused bycheck_data— that the declared numbers are the ones enforced, not numbers both sides happen to store. Both sides of each interval, since they are separately enforceable, and the expected bounds are still written out longhand rather than read fromcomponent_domains, so a wrong constant in the table cannot make the test agree with itself.One test is added: that
coordinaxs.hypothesis'scomponent_domainsis core's object. The merge is only real while that holds — a second table in the strategy package would put the drift straight back.Verification
11658 passed, 311 skipped, 1 xfailed.
prek run --all-filesclean.nox -s docssucceeds.🤖 Generated with Claude Code