Skip to content

refactor(program): a direction carries what a consumer reads, not the check the loader makes - #581

Merged
FBumann merged 2 commits into
claude/mathspec-475-status-3whm7xfrom
claude/mathspec-475-status-3whm7x-surface
Sep 20, 2026
Merged

FBumann merged 2 commits into
claude/mathspec-475-status-3whm7xfrom
claude/mathspec-475-status-3whm7x-surface

Conversation

@FBumann

@FBumann FBumann commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Prompt: "Could we remove some of the things from our public api? Does lpspec need everything? Or could some thing be simpler?" — "Do it as a stacked pr"

Prompt: "Look at 581 — Thats a weird method [_is_single_valued]" — "Yes!" (to inlining it)

Prompt: "This sounds like we could do some cleanup! Try to keep the number of concepts low and cleanly separable" — "Would this simplify the usage in lpspec?" — "all 3"

Note

The following content was generated by AI.

Direction.is_single_valued leaves the public record, and the Predicate union stops claiming something false about itself. Nothing the language accepts, refuses or prints changes.

Stacked on #580.

What this changes

Direction.is_single_valued is a local of the call that refuses on it. It decides between at and sum(by=) at load: at needs the read single-valued and sum needs it not, because a sum landing on the relation's key has one term per group and adds nothing up. One test, used plain and negated, produces both refusals. It had no consumer: a program's node type already says which operator a call became, which is why lpspec never asks. The refusal table in what counts as public API refuses a function whose answer a declaration could give.

It is a local rather than a private helper because _direction already holds every part: shape.key, into_roles and joined are named on the lines above, so a helper taking a Direction would reach back through an object for what the caller just built, and evaluate the same set twice for two refusals on adjacent lines.

The Predicate union's comment said it was "what a lowered mask's root is built of". That is false. ArithmeticComparison is a member, and lowering rewrites every one into an ExpressionComparison, so a lowered mask never holds one. The comment now names the member a program never carries, and the reading page says the same to consumers.

Why the union is not split

The false comment is the honest fix, not the union. assert_never is a static check, so a consumer matching a predicate exhaustively has to write a branch for every member, including one it can never receive. Narrowing the union would spare lpspec two such branches, in linopy/where.py and the polars predicates.py, so I tried it.

It does not stop at the union. Not, And and Or carry Predicate operands and are shared by both stages, so narrowing them propagates into every spec-side caller, and Mask holds a root of one stage or the other. The type checker priced it at eleven errors that only a per-stage copy of the predicate tree resolves. That is a layer, and two branches in one consumer do not buy one.

The audit this came from

Every name in program.__all__, checked for a qualified use in lpspec. lpspec is pinned to alpha.106, so the check ran against the spellings it imports today rather than the ones #580 introduces.

Sixty-nine of eighty-one exported names have one. The surface is not carrying weight it does not need, so nothing else is removed here.

Twelve have no qualified lpspec use, and each is kept for a reason:

Name Kept because
Footprint, QuadraticPosition, TypedPredicate, VariableAbsence, Derivation the declared type of a public field or return value
ExpressionComparison, ArithmeticComparison, Connective, Reach reachable by a consumer walking a mask or a separability verdict
walk_regions documented on the reading page beside walk and children
QUADRATIC_POSITIONS the same shape as DIMENSION_DTYPES, which math_spec.__all__ exports as a vocabulary to pin a table against

I had proposed unexporting the last two before reading tests/test_public_surface.py. It pins program.__all__ to every public name the module defines, so there is no unexported-but-public state: unexporting means making private. Both are documented promises, so both stay.

Gates

pixi.sh is refused by this environment's egress proxy, so the gates ran from a uv environment on Python 3.12 with the pinned ruff==0.16.1 and pyrefly==1.2.0. That is a departure from the "every command runs under pixi run" default.

gate result
pytest -q -n auto 1438 passed
tests/test_public_surface.py passes; __all__ is unchanged, since a property is not a module-level name
ruff check, ruff format --check clean
pyrefly check 0 errors, 9 suppressed, as on the base
mkdocs build --strict clean, with the docs.python.org inventory dropped for the run, which the proxy refuses with a 403
prettier --check on every Markdown clean
schema, goldens, the four page generators no diff
typos, reuse lint, taplo, zizmor, compile-tex not run, for want of the tools

mkdocs build --strict and prettier ran on the first commit, which holds the only Markdown in the diff; the second commit touches src/ alone.

docs/reference/reading.md, by the measurement in the docs-writing skill: n 52, avg 14.9, median 13, over25 7. The long sentences all predate this change; the two added are 11 and 9 words.

No test moved. Nothing read is_single_valued outside the two call sites.

Two claims this body made, corrected

Both were mine, and both were checked only after review had started.

"Moving ArithmeticComparison to _expression_parser would be a runtime import cycle." It would not. program.py already imports that module at runtime, on line 29, for ComparisonOperator. I made the move and ran the suite: 1437 passed, 1 skipped, no cycle.

The conclusion is unchanged, but the reason above is the real one. The move on its own makes matters worse rather than better: the union would still list the member, so a consumer still needs the branch, and could no longer name the type it is matching. The move is only worth making as part of narrowing the union, which is what costs eleven errors.

"lpspec has two such walkers … both ending in assert_never. Narrowing the union would delete those two branches." The branches do not exist. lpspec pins v0.0.0-alpha.106 and has zero references to ExpressionComparison or ArithmeticComparison, because #566, which introduces them, has not landed. Narrowing would spare lpspec from adding two dead branches, not delete two it has. The body above now says so.

That is work the stack owes lpspec and no PR here records: when this lands, both walkers need an ExpressionComparison branch or assert_never stops type-checking.

What this deliberately does not do
  • The Predicate union is not narrowed, for the reason above. The experiment was reverted, not left half-applied.
  • walk_regions and QUADRATIC_POSITIONS stay exported, against my own earlier proposal.
  • ArithmeticComparison stays in program, for the reason in the corrections above: the move is cheap and the narrowing it would have to come with is not.
  • The predicate vocabulary is not reshaped. Eleven atoms encode two axes — what is read, and what is claimed of it — and collapsing them into Defined(read) and Compare(left, op, right) was considered and dropped. A generic Compare makes five combinations representable that the language refuses, so five messages that the node type carries today would have to be rebuilt as guards; the right-hand side is a literal in every form but one, so the symmetry is not there; and both lpspec walkers dispatch on where a value comes from rather than on the comparison, so their branch count does not fall. The where grammar's own overlap, where p > 5 and 1 * p > 5 mean the same and give different trees, belongs to feat(language): a where may compare arithmetic over parameters #566 and is noted there.
  • The branch name is this session's rather than <type>/<topic> from a worktree.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JJfEUsCDtwXuXV8CANCHuR

… check the loader makes

`Direction.is_single_valued` is a module-private function in resolution.py.
It is the discriminator between `at` and `sum(by=)`, it has two callers in
one function, and no consumer asks it: a program's node type already says
which operator a call became. The refusal table in
docs/about/what-counts-as-public-api.md refuses a function whose answer a
declaration could give.

The `Predicate` union said it was what a lowered mask's root is built of.
That is false. It also holds `ArithmeticComparison`, which lowering rewrites
into an `ExpressionComparison`, so a consumer walking a program meets every
other member and never that one. The comment and the reading page now say so.

docs/reference/reading.md: n 52, avg 14.9, median 13, over25 7. The long
sentences all predate this change; the two added are 11 and 9 words.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MKUDdoyCtxHXm5Gcuze24W
@read-the-docs-community

read-the-docs-community Bot commented Sep 20, 2026 •

Copy link
Copy Markdown

@FBumann
FBumann added this pull request to stack #583 September 20, 2026 20:35
…that asks it

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JJfEUsCDtwXuXV8CANCHuR
@FBumann
FBumann removed this pull request from stack #583 September 20, 2026 20:53
@FBumann
FBumann merged commit e5516ae into claude/mathspec-475-status-3whm7x Sep 20, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants