From 667235376c291504cc5594e8b7bfe69b9fe08413 Mon Sep 17 00:00:00 2001 From: FBumann <117816358+FBumann@users.noreply.github.com> Date: Mon, 24 Aug 2026 21:18:59 +0200 Subject: [PATCH] fix: refuse a negated named amount, and one read where there is no coordinate Two of the three rules #62 collected, and the first is why it was filed. `shift(offset=-lead)` loaded clean and then raised a bare `AssertionError` out of the typesetter, because `_amount` matches a unary minus and asserts the operand is a number. That assert was reading a rule the language stated and never enforced --- and it is the rule the whole treatment of a named offset rests on: `walk.py` renders one as always-backward *because* a negated one is refused. It is refused now, at load, in a sentence, so the assert is a precondition rather than a hope. Its docstring said `by=` throughout, which was the kwarg's name two renames ago and is now a different kwarg on the same operator. The second rule is the dim half of the same idea: an amount is read at the coordinate its operator walks, so a parameter varying over a dim that coordinate does not carry has nowhere to be read from. It rendered as a bare symbol, which on the page says "one number" about a column --- the same way an unchecked width did before #58, and wrong in the same direction. `by=` is what makes such a parameter readable again, one amount per group, so the rule admits the dims a partition maps into. That half was `shift`'s alone until #59 gave the windows a partition too; both get it here, and a width per representative day now says what it means. Refs #62 Co-Authored-By: Claude Opus 5 (1M context) --- src/math_spec/dimensions.py | 45 +++++++++++++++++++++++++------ src/math_spec/typesetting/walk.py | 22 ++++++++------- tests/test_dimensions.py | 29 +++++++++++++++++++- 3 files changed, 78 insertions(+), 18 deletions(-) diff --git a/src/math_spec/dimensions.py b/src/math_spec/dimensions.py index a9300c46..4b6e4541 100644 --- a/src/math_spec/dimensions.py +++ b/src/math_spec/dimensions.py @@ -222,7 +222,7 @@ def _dims_call( raise DimensionError( f'{context}: {node.name}(over={over.name}) but the expression has dims {sorted(inner)}.' ) - _check_named_amount(node, over.name, schema, context) + _check_named_amount(node, over.name, inner, schema, context) partition = node.kwargs.get('by') if partition is not None: assert isinstance(partition, LookupNode) @@ -252,6 +252,20 @@ def _dims_call( #: rules below. _AMOUNTS = {'shift': ('offset', 'offset'), 'sum_back': ('within', 'width')} +#: Why negating a named amount at the call site is not what the caller means. +#: ``shift`` has a direction and keeps it in the data; a window's direction is +#: its own name, and a width is a count, which has none. +_NEGATED = { + 'shift': ( + 'A named offset carries its sign in its values, so that one row pointing backwards says ' + 'so where the data is read — negate the column instead.' + ), + 'sum_back': ( + 'A width counts positions and so has no direction; which way a window reaches is the ' + "operator's own name rather than the sign of its width." + ), +} + #: Why a named amount that varies along the axis it walks is not the thing its #: operator claims to be. _VARIES = { @@ -260,19 +274,24 @@ def _dims_call( } -def _check_named_amount(node: FunctionCallNode, over: str, schema: Model, context: str) -> None: - """The two rules that hold of an ``offset=`` or ``within=`` naming a parameter. +def _check_named_amount(node: FunctionCallNode, over: str, inner: frozenset[str], schema: Model, context: str) -> None: + """The rules that hold of an ``offset=`` or ``within=`` naming a parameter. - Both are about the *amount*, not about a dim set, but they live here + They are about the *amount* rather than about a dim set, but they live here because here is where the schema is in hand — a parameter's ``dtype`` and - its ``dims`` are read off the same declaration, and splitting the pair - across two passes would give one rule of a documented pair two voices. + its ``dims`` are read off the same declaration, and splitting a documented + set across two passes would give one rule of it several voices. - A literal is checked by the grammar already: it parses as a number and a - number carries no dims, so only a named amount can break either rule. + A literal breaks none of them: it parses as a number, and a number has + neither a dtype to declare nor dims to vary over. """ kwarg, noun = _AMOUNTS[node.name] amount = node.kwargs[kwarg] + if isinstance(amount, UnaryOperatorNode) and isinstance(amount.operand, ParameterNode): + raise DimensionError( + f'{context}: {node.name}({kwarg}={amount.op}{amount.operand.name}) negates a named ' + f'{noun}. {_NEGATED[node.name]}' + ) if not isinstance(amount, ParameterNode): return declared = schema.parameters[amount.name] @@ -290,6 +309,16 @@ def _check_named_amount(node: FunctionCallNode, over: str, schema: Model, contex f'carries it. A named {noun} that varies along the axis it walks is {_VARIES[node.name]} ' f"— declare '{amount.name}' over dims '{over}' is not one of." ) + partition = node.kwargs.get('by') + groups = frozenset(partition.into) if isinstance(partition, LookupNode) else frozenset() + if stray := sorted(frozenset(declared.dims) - inner - groups): + raise DimensionError( + f'{context}: {node.name}({kwarg}={amount.name}) reads its {noun} at the coordinate it ' + f"walks, but '{amount.name}' varies over {stray}, which that coordinate does not carry " + f'(dims {sorted(inner)}). A dim the coordinate does not have is no coordinate at all — ' + f"declare '{amount.name}' over dims the expression carries, or group by a lookup into " + f'one of {stray}, so that each group is reached by its own {noun}.' + ) # --------------------------------------------------------------------------- diff --git a/src/math_spec/typesetting/walk.py b/src/math_spec/typesetting/walk.py index 2954b372..fd388cba 100644 --- a/src/math_spec/typesetting/walk.py +++ b/src/math_spec/typesetting/walk.py @@ -85,15 +85,19 @@ def _amount(node: ArithmeticNode) -> int | str: - """``shift``'s ``by=``: a signed number, or the name of a parameter. + """``shift``'s ``offset=``: a signed number, or the name of a parameter. A negated literal parses as a unary minus over a number rather than as a - negative one, so reading the ``NumberNode`` alone both aborted on ``by=-1`` - and left the forward direction of every translation operator unreachable. - - A named offset comes back as its name. It is always backward — the language - refuses ``by=-p``, so the direction lives in the data — and it renders as - the parameter's own symbol rather than as a number. + negative one, so reading the ``NumberNode`` alone both aborted on + ``offset=-1`` and left the forward direction of every translation operator + unreachable. + + A named offset comes back as its name. It is always backward, because a + negated one is refused at load — :func:`math_spec.dimensions.check_schema`, + so the direction lives in the data — and it renders as the parameter's own + symbol rather than as a number. The assert below is that rule's + precondition rather than a hope: before it was enforced, this raised a bare + ``AssertionError`` out of the typesetter on a model that had loaded clean. """ if isinstance(node, ParameterNode): return node.name @@ -241,8 +245,8 @@ def translation(self, step: _Step, group: str = '') -> str: the group the translation stays inside. """ backward, forward = _TRANSLATIONS[step.policy] - # a named offset is always backward: `by=-p` is refused, so the sign is - # in the data and the operator cannot read it off the call + # a named offset is always backward: `offset=-p` is refused at load, so + # the sign is in the data and the operator cannot read it off the call operator = self.op(backward if isinstance(step.by, str) or step.by > 0 else forward) if step.fill: operator = self.format.subscript(operator, [step.fill]) diff --git a/tests/test_dimensions.py b/tests/test_dimensions.py index 501998ff..1b32a56f 100644 --- a/tests/test_dimensions.py +++ b/tests/test_dimensions.py @@ -31,7 +31,11 @@ 'generator': {'values': ['wind', 'gas']}, 'bus': {'values': ['n', 's']}, }, - 'lookups': {'gen_bus': {'over': 'generator', 'into': 'bus'}}, + 'lookups': { + 'gen_bus': {'over': 'generator', 'into': 'bus'}, + # over `snapshot`, so it can partition the axis the translations walk + 'snap_bus': {'over': 'snapshot', 'into': 'bus'}, + }, 'parameters': { 'p_max': {'dims': ['generator']}, 'cost': {'dims': ['generator']}, @@ -40,6 +44,8 @@ # need a parameter that obeys them and one that spans the axis walked 'spinup': {'dims': ['generator'], 'dtype': 'int'}, 'horizon': {'dims': ['snapshot'], 'dtype': 'int'}, + # over a dim `p` does not carry, so it is readable only through a `by=` + 'bus_lead': {'dims': ['bus'], 'dtype': 'int'}, }, 'variables': {'p': {'foreach': ['snapshot', 'generator'], 'bounds': {'lower': 0, 'upper': 'p_max'}}}, 'constraints': { @@ -86,6 +92,9 @@ def test_the_base_model_typechecks(): ("shift(p, over=snapshot, offset=1, edge='wrap')", {'snapshot', 'generator'}), ("shift(p, over=snapshot, offset=spinup, edge='wrap')", {'snapshot', 'generator'}), ('sum_back(p, over=snapshot, within=spinup)', {'snapshot', 'generator'}), + # the same offset a `by=` makes readable: one lag per group it maps into + ("shift(p, over=snapshot, offset=bus_lead, edge='wrap', by=snap_bus)", {'snapshot', 'generator'}), + ('sum_back(p, over=snapshot, within=bus_lead, by=snap_bus)', {'snapshot', 'generator'}), ], ) def test_dim_inference(expr, expected): @@ -157,6 +166,24 @@ def test_dim_inference(expr, expected): r'no longer "the last n"', id='a-named-width-does-not-span-the-summed-axis', ), + # Before this was refused, it loaded clean and then raised a bare + # `AssertionError` out of the typesetter (#62) — on the very rule + # `walk.py` reads a named offset as always-backward *because of*. + pytest.param( + "shift(p, over=snapshot, offset=-spinup, edge='wrap')", + r'negates a named offset', + id='a-named-offset-is-not-negated-at-the-call', + ), + pytest.param( + 'sum_back(p, over=snapshot, within=-spinup)', + r'which way a window reaches is the operator', + id='a-named-width-has-no-direction-to-negate', + ), + pytest.param( + "shift(p, over=snapshot, offset=bus_lead, edge='wrap')", + r"varies over \['bus'\], which that coordinate does not carry", + id='a-named-offset-is-read-where-the-expression-has-a-coordinate', + ), ], ) def test_an_ill_dimensioned_expression_is_rejected(expr, match):