fix(#816): Expression shape/len/iter protocol + concatenate/stack - #820
Merged
Conversation
Bug (the urgent part): ``Variable`` supported ``__getitem__`` but not ``__len__``/``__iter__``, and out-of-range indexing returned an expression instead of raising. Python then fell back to the legacy sequence-iteration protocol (``x[0]``, ``x[1]``, … until ``IndexError``), which never terminated — ``list(x)``/``sum(x)``/``np.array(x)`` hung a process on any accidental iteration. - Out-of-range integer indexing on a *shaped* (ndim >= 1) expression now raises ``IndexError`` at build time (numpy semantics), via a zero-allocation broadcast probe that mirrors the JAX compiler's ``base[index]``. Scalar-base indexing (``x[0]`` on shape ``()``) keeps its pre-existing leniency — #816 is about array indexing, and the LLM model builder relies on scalar indexing. - ``Expression`` gains ``__len__`` (leading-axis length) and ``__iter__`` (yields ``expr[0]``, ``expr[1]``, …); both raise ``TypeError`` for scalar or shape-unknown expressions instead of hanging. Enhancement: ``.shape`` now propagates through indexing/slicing, element-wise ``BinaryOp``/``UnaryOp`` and element-wise ``FunctionCall`` (``sqrt``, ``exp``, …). Exposed as a read-only ``Expression.shape`` property that raises ``AttributeError`` when the shape is not statically known, so ``getattr(expr, "shape", default)`` / ``hasattr`` keep their prior behaviour for shape-unknown nodes (reductions, matmul, custom). ``dm.concatenate`` / ``dm.stack`` assemble a vector of scalar expressions from pieces (method-of-lines "boundary, interior, boundary" pattern) as a numpy object ndarray — no new DAG node type, each element is an ordinary scalar expression the solver already handles. Adds python/tests/test_expression_shape_iter_816.py (17 cases: OOB IndexError 1-D/2-D, len/iter, no-hang builtins, scalar-indexing leniency, shape propagation, concatenate/stack, and the issue's shared momentum-residual reused verbatim across numpy and discopt backends). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbM6bykXYYunrv3fygbzh5
All 31 are latent under ``warn_return_any``; unrelated to any behaviour.
- mip_nlp_rootsearch: ``VectorLike`` module alias annotated with
``TypeAlias`` so it is valid in annotation position (12 valid-type).
- mccormick_nlp / solver: ``return -np.inf`` -> ``return float("-inf")``
where the enclosing function is declared ``-> float`` (``np.inf`` types
as ``Any`` in the installed numpy stubs); value is identical (15
no-any-return).
- mcbox / ellipsoidal_arith: wrap ``.shape[0]`` / ``.size`` in ``int()``
for the ``-> int`` properties (2).
- sparsity / solver: wrap numpy comparisons in ``bool()`` for the
``-> bool`` returns (2).
``mypy python/discopt/`` now reports 0 errors.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HbM6bykXYYunrv3fygbzh5
…pression lenient) CI surfaced a documented internal contract the eager guard broke: the expression-graph machinery (GAMS import, NL export resolver, signomial / sign-domain pattern matching) constructs ``IndexExpression(base, idx)`` with out-of-range, non-integer, and too-many-index values as lazy nodes it resolves conservatively later — e.g. test_sign_domain_checks: ``bad = IndexExpression(v, 10) # must be conservative, not raise``. Fix by layering: - ``IndexExpression.__init__`` / ``_index_result_shape`` are now purely best-effort shape inference and NEVER raise; any index numpy cannot resolve yields shape-unknown. - The out-of-bounds IndexError moves to ``Expression.__getitem__`` (the user-facing ``[]`` operator) and fires only for a plain integer or a full-arity all-integer tuple that is out of range — the ``x[99]`` / ``X[i, j]`` typo the issue names. Slices, ellipsis, wrong-arity tuples, and non-integer keys stay lenient (the lazy internal forms). Fixes the 7 CI failures (test_export_cli_roundtrip, test_gams ×4, test_relax_compiler_convexity_units ×2) while preserving the issue's user-facing guard; regression suite unchanged and green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbM6bykXYYunrv3fygbzh5
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 #816.
Two behaviors on modeling
Expressionobjects.Bug (the urgent part).
Variablesupported__getitem__but not__len__/__iter__, and out-of-range indexing returned an expression instead of raising. Python then fell back to the legacy sequence-iteration protocol (x[0],x[1], … untilIndexError), which never terminated —list(x)/sum(x)/np.array(x)wedged the process on any accidental iteration.Expressiongains__len__(leading-axis length) and__iter__(yieldsexpr[0],expr[1], …); both raiseTypeErrorfor scalar or shape-unknown expressions instead of hanging.__iter__is what actually kills the hang (Python no longer uses the sequence protocol).[]operator on a shaped (ndim ≥ 1) expression now raisesIndexError(numpy semantics) — for a plain int or a full-arity all-int tuple that is out of range (x[99],X[i, j]). The guard lives onExpression.__getitem__; directIndexExpression(base, idx)construction stays conservative (never raises), because the expression-graph internals (GAMS import, NL export resolver, signomial/sign-domain matching) build out-of-range / non-integer / too-many-index nodes as lazy forms resolved downstream (test_sign_domain_checks:IndexExpression(v, 10)"must be conservative, not raise"). Scalar-base indexing (x[0]on shape()), slices, wrong-arity tuples, and non-integer keys stay lenient.Enhancement.
.shapenow propagates through indexing/slicing, element-wiseBinaryOp/UnaryOp, and element-wiseFunctionCall(sqrt,exp, …), exposed as a read-onlyExpression.shapeproperty that raisesAttributeErrorwhen the shape is not statically known — sogetattr(expr, "shape", default)/hasattrkeep their prior behaviour for shape-unknown nodes (reductions, matmul, custom).dm.concatenate/dm.stackassemble a vector of scalar expressions from pieces (the method-of-lines "boundary, interior, boundary" pattern) as a numpy object ndarray — no new DAG node type, each element is an ordinary scalar expression the solver already handles.The issue's shared
momentum_residualnow reuses verbatim acrossnp.sqrtanddm.sqrt(a regression test pins this). Request #3 (np.sqrt(expr)via__array_ufunc__) is intentionally not in scope: it would require replacing the load-bearing__array_ufunc__ = None(which defersA @ xto__rmatmul__) with a handler that re-owns all ufunc dispatch including matmul — a correctness-sensitive marshaling change that belongs in its own bound-neutral issue.Second commit (
chore(types), unrelated to #816, requested in review) clears 31 pre-existingmypyerrors in the solver stack (TypeAliasonVectorLike;-np.inf→float("-inf")for-> floatreturns;int()/bool()wraps).mypy python/discopt/now reports 0 errors.Correctness
No solver-math change: this is modeling-DSL metadata and Python-side iteration. The out-of-bounds guard only makes a provably invalid user-facing index (
x[99]) fail loudly instead of building a silently-invalid node; it never rejects a valid model, and internal lazyIndexExpressionconstruction is untouched (stays conservative)..shapeinference is exact (numpy broadcast rules / the probe mirrors the compiler'sbase[index]) and never raises, so the M8 build-time shape guard fires only on genuinely incompatible shapes.getattr(con.body, "shape", ())and similar consumers are unaffected (AttributeError preserves the default). End-to-end solves with shaped/sliced/sqrtexpressions verified (objective correct).Tests run (state the result — numbers, not adjectives)
pytest -m smoke→ 817 passed, 15 skipped, 2 xpassedPython fastCI lane green onef889ae(an earlier revision failed 7 tests — GAMS/export/signomial/sign-domain — because the guard first sat inIndexExpression.__init__and broke internal lazy construction; moving it to__getitem__fixed all 7)pytest -m slow python/tests/test_adversarial_recent_fixes.py→ 9 passed, 1 flaky (test_large_dense_jacobian_no_crashtrips its wall-clock deadline under load; passes standalone in 27s, scalar-only, unrelated to this change)cargo test -p discopt-core→ N/A (no Rust touched)ruff check/ruff format --checkclean;mypy python/discopt/→ 0 errorsRegression test
python/tests/test_expression_shape_iter_816.py(17 cases): OOBIndexError1-D/2-D via[], scalar-indexing leniency,len/iter, no-hang builtins (list/sum/np.array),.shapepropagation,concatenate/stack, and the issue's shared momentum-residual reused verbatim across numpy and discopt backends. Each fails before the change (hang / silent no-op /AttributeError) and passes after.Generated by Claude Code