Variable.update() / Constraint.update() as canonical mutation API#727
Conversation
Introduces typed ``.update()`` methods on Variable and Constraint as the single, validated, multi-attribute mutation entry point. - ``Variable.update(lower=, upper=)``: validates non-constant inputs are rejected, new dims are rejected, and the resulting ``lower <= upper`` invariant holds across all coords. Returns ``self`` for chaining. - ``Constraint.update(rhs=, sign=)``: constant RHS only. The legacy ``c.rhs = variable`` rearrange-to-lhs path stays on the setter (different semantic, deserves its own explicit method). The existing ``.lower`` / ``.upper`` / ``.sign`` setters become thin shims that forward to ``.update()``, so single-attribute writes (``z.lower = 2``) stay ergonomic and the canonical validation runs in one place. The ``.rhs`` setter forwards constants through ``.update()`` and keeps the expression-rhs rearrange behaviour. This is the on-top experiment for the design discussion on #718. ``.lhs`` / ``.coeffs`` / ``.vars`` setters are untouched for now. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the existing ``c.rhs = expr`` setter and ``add_constraints`` which both accept mixed-side input and rearrange the residual onto lhs. ``c.update(rhs=x + 5)`` now subtracts ``x`` from lhs and stores ``5`` on rhs. ``.rhs`` setter collapses to a one-line shim. Variable bound rejection of Variable/Expression is kept (bounds are numeric, not symbolic); docstring clarified to spell out that pandas / xarray / numpy arrays are first-class (time-varying bounds). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…etters Adds lhs / coeffs / vars to the canonical mutation API. All .lhs / .coeffs / .vars setters now forward to .update() — every Constraint mutation goes through one method with one validation path, one place that flips _coef_dirty. Composition rules: - lhs= replaces the whole expression first; subsequent rhs= rearrangement (Variable/Expression in rhs) sees the new lhs. - lhs= and coeffs= / vars= are mutually exclusive (whole replacement vs partial array update). - sign= is applied last so it composes cleanly. Internal Constraint.sanitize_zeros migrated to update(vars=, coeffs=) — no more internal setter calls in linopy/. 389 tests pass across mutation + persistent-solver suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Avoids shadowing Python's vars() builtin. The .vars attribute on Constraint stays (it parallels the .data.vars internal name); only the kwarg gets the unambiguous spelling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This is much closer to how JuMP does things. We have
In MathOptInterface we have a CachingOptimizer which controls the "pass to solver OR rebuild" logic:
I tried to cover the caching optimizer in my MOI talk, but I don't really like it. It was too brief and I did things in the wrong order: https://youtu.be/M31xoZGyj9w?si=NTQHUsnq7o9Lg770&t=2211 |
Mirrors add_constraints' dispatch: c.update(x + 5 <= 3) is now shorthand for c.update(lhs=x, sign='<=', rhs=-2), extracted from the AnonymousConstraint / ConstraintBase the comparison produces. Mutually exclusive with the per-attribute kwargs; clear error when mixed. Also reverts the internal sanitize_zeros migration. The setters are pure shims forwarding to update(), so the migration didn't change behaviour or cost — just spelling. The original setter syntax reads more naturally there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The positional ConstraintLike form (c.update(x + 5 <= 3)) always rewrites lhs / sign / rhs and flips _coef_dirty. For hot loops that only touch one part, kwarg form (c.update(rhs=...)) skips the unchanged attributes and is materially cheaper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up: what
|
Closes the A1 residual from the #718 review. The flag-trust path (`skip_coef_compare = same_model and not coef_dirty`) is correct through Constraint.update() (set in one place, shims forward), but `c.coeffs.values[...] = ...` still bypasses _coef_dirty. With same_model=True as the default, that bypass silently produces wrong diffs. Flip the default to False. Cross-model paths (the only production caller, Solver._update_locked, passes explicitly) are unaffected. Same-model warm-update paths now value-diff the CSR data — small perf hit (50-200ms at Mayk-scale per Mayk's bench), correct by default. Solver-aware callers who own the mutation contract can opt back into the optimization with `same_model=True`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- examples/manipulating-models.ipynb: rewrite mutation cells to use Variable.update / Constraint.update; setter form is mentioned in notes as syntactic sugar for the same call. - examples/creating-constraints.ipynb: reframe the CSRConstraint vs Constraint API table around .update() as the mutation API; setters are sugar. - Setter docstrings now say 'syntactic sugar for Constraint/Variable .update; do not add logic here so the contract stays single-sourced' — a directive to future contributors as much as to readers. No deprecation, no breaking change. .update() is the documented canonical mutation API; the seven setters continue to exist as one-line shims. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
API surface from a user perspectiveTying this back to the Two follow-ups worth committing to here:
End state: users call |
Adds DeprecationWarning to all seven mutation setters (Variable.lower, Variable.upper, Constraint.coeffs, Constraint.vars, Constraint.sign, Constraint.rhs, Constraint.lhs). Each setter still forwards to .update() so existing code keeps working; the warning points at the canonical API. Internal sanitize_zeros migrated off setters (the last linopy/ caller). api.rst gains Modification sections listing .update() for both Variable and Constraint; tutorial notes rewritten to teach .update() and flag setters as deprecated. Release note added. dual.setter / solution.setter untouched — result assignment, not mutation, different deprecation track. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
With
This results in a clear split:
Users do updates, we push DIffs internally to the solver. |
Constraint.update tests: lhs-only, coeffs-only (vars preserved), compound lhs+sign, mutually-exclusive lhs+coeffs and lhs+variables. Variable.update tests: upper-only, valid array bound. Migrate test_constraint_coef_dirty.py from the now-deprecated setters to .update(), exercising the canonical path; add positional-form and no-op cases. Net effect: same dirty-flag invariants, 7 fewer warnings per pytest run. Docs: Constraint.update rhs= gains a worked example showing the two forms (constant vs variable/expression). add_constraints rhs gets a matching note pointing at the linopy invariant so the rearrangement rule is documented at the creation site too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
I like that very much. let's do that. we can also make that another pr or adjust it directly in #718 - does not need to be added here. For now, I would like to keep the setters and not deprecate them. fine if I adjust that here? |
|
I you know what, let's deprecate them :) |
|
@FabianHofmann As you wish :) SO its softer than a FutureWarning. Mostly a notice in the release notes. |
| rhs: ExpressionLike | VariableLike | ConstantLike | None = None, | ||
| sign: SignLike | None = None, | ||
| coeffs: ConstantLike | None = None, | ||
| variables: variables.Variable | DataArray | None = None, |
There was a problem hiding this comment.
let's add a deprecation warning if dataarray is passed as variables. we should only support variable type in future.
There was a problem hiding this comment.
Or even a Future Warning? Is this currently a use case?
| if variables is not None: | ||
| from linopy.variables import Variable as _Variable | ||
|
|
||
| v = variables.labels if isinstance(variables, _Variable) else variables |
There was a problem hiding this comment.
somewhere here would be the deprecation warning I mentioned above
| non_constant = ( | ||
| Variable, | ||
| ScalarVariable, | ||
| expressions.LinearExpression, | ||
| expressions.QuadraticExpression, | ||
| ) |
There was a problem hiding this comment.
use ConstantLike from types.py to test
There was a problem hiding this comment.
I would suggest to add a function self._validate_update(lower=None, upper=None) which checks for correct type and dimensions in a for loop. after validation they can be broadcasted, added and finally checked (final_lower < final_upper) here
There was a problem hiding this comment.
So _validate_update would NOT check (final_lower < final_upper) ?
sounds very good |
- Constraint._assign_lhs_expr → _assign_lhs (drop redundant suffix; the method already takes a LinearExpression, so the type was in the signature, not the name). - Add Constraint._assign_data(**fields) helper. Wraps the four ``self._data = assign_multiindex_safe(self.data, **kw)`` callsites inside update() (rhs / coeffs / vars / sign). Untouched: the same pattern in dual.setter, sanitize_missings, sanitize_infinities — those aren't update() and stay out of scope here. - Add types.CONSTANT_TYPES tuple, derived from ConstantLike via get_args so the two cannot drift. Variable.update bound validation flipped from negative (isinstance against a hand-rolled non_constant tuple) to positive (isinstance against CONSTANT_TYPES); drops a redundant in-function ``from linopy import expressions`` (the module-level import already covered it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@FabianHofmann
|
sorry, got swallowed by the other prs. yes and futurewarning |
…_validate_update Constraint - sanitize_zeros now writes _data via _assign_data directly (no longer round-trips through update(variables=DataArray), which would self-trigger the new deprecation warning). - Constraint.update(variables=...) emits FutureWarning when passed a raw DataArray of integer labels; Variable is the supported input. The path stays accepted for back-compat and will be removed alongside the v1 cleanup. Variable - Extract Variable._validate_update(*, lower, upper) — validates, broadcasts, and runs the cross-bound (lb<=ub) check, returning a dict ready for assignment. update() body shrinks to ~3 lines. Coord validation (parity with add_variables) deferred to #726 land. Tests - test_constraint_coef_dirty's variables= test now passes a Variable instead of a DataArray (matches the supported input). - test_constraint_vars_setter_with_array wrapped in pytest.warns(FutureWarning) — locks in the deprecation contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…y inside The helper now writes fields AND flips _coef_dirty when the written set includes coeffs or vars. Callsites in update() (coeffs / vars branches) and sanitize_zeros drop their explicit `self._coef_dirty = True` lines — the rule lives in one place, can't be forgotten by future field additions. rhs / sign writes still don't dirty (correctly). _assign_lhs is untouched: it uses a different write mechanic (drop_vars + assign) and manages its own flag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stacked on #718. Exploratory branch for the API discussion (#718 review thread).
Adds typed
.update()methods onVariableandConstraintso every mutation goes through one validated entry point. The existing 7 setters survive as one-line shims that forward to.update(), so no user-visible breakage — but_coef_dirty, broadcasting, sign normalisation, and the rhs-rearrange-to-lhs logic all live in one method.API
Two call shapes for Constraint, mirroring
add_constraints:Composition rules:
lhs=replaces the whole expression first;rhs=rearrangement then sees the new lhs.lhs=is mutually exclusive withcoeffs=/variables=(whole vs partial).constraintis mutually exclusive with all keyword arguments.sign=applied last so it composes cleanly.selffor chaining.Setter shims
Forwarders, no other behaviour:
var.lower = xvar.update(lower=x)var.upper = xvar.update(upper=x)c.lhs = exprc.update(lhs=expr)c.rhs = …c.update(rhs=…)c.sign = sc.update(sign=s)c.coeffs = xc.update(coeffs=x)c.vars = vc.update(variables=v)Closes the #718 review A1 residual
Last commit (
70dbed4) flipsModelDiff.from_snapshot(same_model=…)default fromTruetoFalse. The flag-trust path (skip_coef_compare = same_model and not coef_dirty) is now precise throughConstraint.update()— it sets the flag in one place; setters forward there. Butc.coeffs.values[...] = ...still bypasses_coef_dirty, and with the old default, that bypass silently produced wrong diffs.The only production caller (
Solver._update_locked) passessame_modelexplicitly, so it's unaffected. Same-model warm-update paths now value-diff the CSR data instead of trusting the flag — small perf cost (50–200 ms at Mayk-scale per his bench), correct by default. Solver-aware callers who own the mutation contract can opt back into the optimization withsame_model=True.What's NOT in this branch
Variable.update(type=…)for binary/integer/continuous switching (would need new validation logic).Objective.update(...)(separate container; the persistent-solver diff already tracksobj_linear/obj_sense).sanitize_zerosand friends). Setters are pure shims so the migration would be cosmetic; postpone until / unless setters get actually removed.assign_multiindex_safecalls a multi-attr update makes.Warts (inherited, not introduced)
variables=kwarg shadows the.varsattribute spelling — picked the unambiguous name to dodge Python'svars()builtin shadow;.vars(read) stays as the attribute.rhs=Variable/Expressionsilently rearranges onto lhs (kept for symmetry withadd_constraints; documented).lhs=xorcoeffs=/variables=is a runtime check, not a type-system constraint.Test plan
test_variable.py,test_constraint.py,test_constraints.py,test_constraint_coef_dirty.py,test_model.py,test_variables.py— 280 pass (266 existing + 14 new for.update()directly, incl. the positionalConstraintLikeform).test_persistent_snapshot_diff.py,test_persistent_snapshot_buffers.py,test_persistent_solver_extras.py,test_persistent_solver_orchestrator.py,test_persistent_apply_update.py— 112 tests pass on this branch with thesame_model=Falsedefault.🤖 Generated with Claude Code