Bind a regular commitment to its commodity's aggregate, not each device (fixes #2379) - #2380
Conversation
… device Fixes #2379 (unreleased regression from #1946). convert_to_commitments emitted one FlowCommitment per device (device=d, from re-enumerating the raw flex-model list), so a regular commitment held each device to the baseline individually. Bind one commitment per commodity over all its devices instead (device=<commodity's inventory indices>, device_group=commodity), mirroring the internal '<commodity> net energy' commitment; this also drops the fragile raw-flex-model enumeration in favour of the device inventory. The now-unused flex_model parameter is removed; the direct-convert tests set device_inventory and assert the aggregate device set. Adds a two-devices-of-one-commodity regression test (the combined flow reaches a baseline neither device could carry alone). Distinct from #2326/#2355 (that is the solver's EMS-level device=None constraint being unbound, affecting direct device_scheduler callers); this path uses device=<list>, so it does not go through that code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TaMepbxRYzJxQKFtu6Hq4p Signed-off-by: F.N. Claessen <felix@seita.nl>
Documentation build overview
3 files changed± changelog.html± _autosummary/flexmeasures.data.models.planning.storage.html± api/v3_0.html |
The regular-commitment aggregation regression was introduced by the multi-commodity work; append this PR to that existing changelog entry rather than adding a new one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TaMepbxRYzJxQKFtu6Hq4p Signed-off-by: F.N. Claessen <felix@seita.nl>
There was a problem hiding this comment.
Pull request overview
Fixes regression #2379 where an unscoped (regular) flex-context commitment was incorrectly bound per-device instead of binding the aggregate flow across all devices of the commitment’s commodity, aligning user commitments with the scheduler’s internal “<commodity> net energy” aggregate commitment behavior.
Changes:
- Update
StorageScheduler.convert_to_commitmentsto create one aggregateFlowCommitmentper unscoped commitment, binding the commodity’s full device set viaself.device_inventory.commodity_to_devices(and drop the now-unusedflex_modelparameter). - Adjust unit tests to initialize
device_inventoryexplicitly for directconvert_to_commitmentscalls and assert against aggregate device sets. - Add regression test covering a two-device same-commodity scenario that distinguishes aggregate binding from per-device binding.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
flexmeasures/data/models/planning/storage.py |
Fix unscoped commitment conversion to bind commodity-aggregate devices using canonical device inventory indices; remove flex_model parameter. |
flexmeasures/data/models/planning/tests/test_commitments.py |
Update direct-conversion tests to set device_inventory, and add regression coverage ensuring unscoped commitments bind the commodity aggregate. |
Two existing tests encoded the pre-#2379 per-device commitment behaviour: - test_flex_context_commitments_target_devices_not_stock_only_entries: a regular commitment now yields a single aggregate commitment binding the scheduled devices (indices 0 and 1), not one commitment per raw flex-model entry. The stock-only exclusion it guards against is unchanged. - test_create_simultaneous_jobs: the sample commitment rewarding supply binds the site aggregate, so it stays inactive while the site is net-consuming and no longer biases the EV/battery split (EV costs 2.3125 -> 2.2375). Total cost is unchanged, matching the fixture's stated intent that the commitment not affect the schedule. Signed-off-by: F.N. Claessen <felix@seita.nl>
|
CI caught two existing tests that encoded the pre-fix per-device commitment behaviour; updated both to the aggregate semantics this PR restores (
Both verified locally before pushing. |
| # Aggregate (unscoped) commitment semantics (issue #2379): the sample commitment | ||
| # rewarding supply binds the site's *aggregate* flow, so it stays inactive while | ||
| # the site is net-consuming and does not bias the per-device dispatch. Under the | ||
| # earlier per-device binding it wrongly rewarded the battery's supply, shifting the | ||
| # EV/battery split (EV costs were €2.3125); the total cost is unchanged either way. |
There was a problem hiding this comment.
Finally we understand why these shares kept changing.
The bug being fixed here (from PR FlexMeasures#1946) has not been released: it sits in the unreleased v1.0.0 section. A separate Bugfixes entry would tell readers about a regression they were never exposed to, so this appends PR FlexMeasures#2355 to the existing multi-commodity feature entry instead, as PR FlexMeasures#2380 did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen <claessen@seita.nl>
Giving ems_flow_commitment_equalities bounds exposed an overlap with grouped_commitment_equalities that did not matter while the rows were free. PR FlexMeasures#2380, merged after this PR was opened, binds a regular flex-context commitment to the aggregate of its commodity's devices by giving it a "device" and "device_group". Such a commitment is therefore already bound, once per device group. Binding it here as well constrains the same commitment_downwards_deviation/commitment_upwards_deviation variables a second time, against a different device set, which over-constrains the problem: 9 device-, group- and stock-scoped commitment tests failed, including the one PR FlexMeasures#2380 added. Skip commitments that have a device group, so this constraint family applies only to commitments naming no device -- the EMS-level case this PR is about. The test added here still exercises that case, as an EMS-level FlowCommitment gets no device_group_lookup entry. Co-authored-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen <claessen@seita.nl>
* fix(scheduling): bind EMS-level flow commitments EMS-level commitments currently create an unbounded constraint tuple, leaving their deviation variables uncoupled from device flow. Mirror grouped commitments when selecting bounds and cover aggregate scheduling with a regression test. Signed-off-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com> * Do not bind a device-scoped commitment at EMS level as well Giving ems_flow_commitment_equalities bounds exposed an overlap with grouped_commitment_equalities that did not matter while the rows were free. PR #2380, merged after this PR was opened, binds a regular flex-context commitment to the aggregate of its commodity's devices by giving it a "device" and "device_group". Such a commitment is therefore already bound, once per device group. Binding it here as well constrains the same commitment_downwards_deviation/commitment_upwards_deviation variables a second time, against a different device set, which over-constrains the problem: 9 device-, group- and stock-scoped commitment tests failed, including the one PR #2380 added. Skip commitments that have a device group, so this constraint family applies only to commitments naming no device -- the EMS-level case this PR is about. The test added here still exercises that case, as an EMS-level FlowCommitment gets no device_group_lookup entry. Co-authored-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen <claessen@seita.nl> --------- Signed-off-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> Co-authored-by: F.N. Claessen <claessen@seita.nl> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The dispatch to the direct HiGHS backend listed its keyword arguments by hand. That is a trap for the branches currently adding scheduling parameters: whoever adds the next one (coupling_groups in #2218, balance_groups in #2289) works on the Pyomo model further down the file, and a parameter missing from the dispatch list would not fail. It would simply never reach the backend, producing a schedule computed as if the constraint had never been requested -- and since this PR makes "highspy" the default solver, that would be silently wrong. Forward by name instead, mapping device_scheduler's signature onto the backend's. An argument the backend does not model raises NotImplementedError naming it, but only when the caller actually set it, so leaving a future parameter at its default stays free. The signature comparison is cached on the two function objects (~70 us once per process, 2 us per call after). Also record, in the highspy module docstring, that the deliberate omission of ems_flow_commitment_equalities stops being harmless once #2355 gives those rows bounds, and that #2380 routes unscoped flex-context commitments through grouped_commitment_equalities (which this backend does build). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen <claessen@seita.nl>
…n) (#2364) * feat: direct HiGHS (highspy) implementation of the device scheduler Add flexmeasures/data/models/planning/highspy_optimization.py, which builds the device scheduler's LP/MILP directly with the HiGHS Python API (highspy), bypassing Pyomo's model construction and solution-ingestion overhead. The Pyomo implementation (linear_optimization.device_scheduler) remains the semantic reference; the module docstring prominently documents that the two models must be kept in sync, as well as the (verified) deviations: - ems_flow_commitment_equalities are not built: on the Pyomo path they are bound-less, i.e. free rows without any effect on the solution - rows no finite assignment can satisfy (bounds involving +/-inf quantities) are skipped, mirroring HiGHS rejecting such rows when appsi adds them - solver results and model are lightweight shims exposing the attributes callers consume (termination_condition/status strings, commitment_costs, commodity_costs, costs, and indexed variable views) The model is built with vectorized numpy arrays (addVars/addRows/ changeColsCost/changeColsIntegrality), constructing and solving typical battery problems in milliseconds, where the Pyomo layer needs seconds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtuVTVfL4fQ9QSqbLXmAGD Signed-off-by: F.N. Claessen <claessen@seita.nl> * feat: expose the direct HiGHS backend as solver choice "highspy" When FLEXMEASURES_LP_SOLVER is set to "highspy", device_scheduler delegates to device_scheduler_highspy with the same inputs and the same return contract. All other solver names keep using the Pyomo path unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtuVTVfL4fQ9QSqbLXmAGD Signed-off-by: F.N. Claessen <claessen@seita.nl> * tests: prove equivalence of the highspy and Pyomo scheduler backends - Add "highspy" to the app_with_each_solver fixture params, so tests using it also run against the direct backend. - Add test_highspy_equivalence.py, running representative scenarios (battery with prices; soc targets incl. storage efficiency and stock delta; site capacity with breach and peak prices; two devices with a StockCommitment; an infeasible case) through both appsi_highs and highspy, asserting near-identical schedules and costs, and matching termination handling. - Make test case 2 of test_multiple_devices_simultaneous_scheduler assert solver-independent properties (aggregate schedule, total costs, total unmet demand): the problem has multiple optima, and only the site-level schedule is unique, while the per-device slot allocation is an arbitrary tie-break that depends on the solver backend. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtuVTVfL4fQ9QSqbLXmAGD Signed-off-by: F.N. Claessen <claessen@seita.nl> * feat: make the direct HiGHS backend the default LP solver Flip the FLEXMEASURES_LP_SOLVER default from "appsi_highs" to "highspy" and update the configuration, installation and deployment docs accordingly. Any Pyomo-based solver remains available as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtuVTVfL4fQ9QSqbLXmAGD Signed-off-by: F.N. Claessen <claessen@seita.nl> * docs: point the changelog entry at the actual PR number Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtuVTVfL4fQ9QSqbLXmAGD Signed-off-by: F.N. Claessen <claessen@seita.nl> * perf: vectorize the subcommitment conversion Hoist convert_commitments_to_subcommitments to module level (it is solver-agnostic and closure-free) and share it between both scheduler backends, removing the duplicated copy from the highspy module. Splitting a commitment into per-group subcommitments now uses a single groupby pass (in order of first appearance, like pd.unique) instead of filtering the DataFrame once per group, and the price non-uniqueness checks are vectorized across all groups. This removes a cost that scaled quadratically with the number of time steps (each time step often forms its own group), benefiting both backends: a 2-device, 192-step benchmark drops from 0.47s to 0.28s via appsi_highs and from 0.37s to 0.12s via highspy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtuVTVfL4fQ9QSqbLXmAGD Signed-off-by: F.N. Claessen <claessen@seita.nl> * review: address Copilot comments on solver output and solver docs - Do not disable HiGHS output unconditionally in the direct backend; only force output_flag false when LOGGING_LEVEL is "INFO", mirroring exactly how the Pyomo path builds its solver options profile, so verbose logging modes can still see solver output. Operator-configured FLEXMEASURES_LP_SOLVER_OPTIONS are still applied last and can override. - Clarify in the configuration docs that a separate solver installation is only needed for external solvers such as cbc; both HiGHS-based choices (highspy and appsi_highs) rely on the bundled highspy package. - Remove the now-inconsistent "pip install highspy" instruction from the deployment docs, which already state that highspy ships with FlexMeasures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtuVTVfL4fQ9QSqbLXmAGD Signed-off-by: F.N. Claessen <claessen@seita.nl> * review: remove dead commodity_devices lookup from the direct backend The commodity -> device indices lookup was only consumed by the Pyomo path's ems_flow_commitment_equalities, which the direct backend deliberately does not build (they are free rows). A breadcrumb comment keeps pointing readers at that documented skip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtuVTVfL4fQ9QSqbLXmAGD Signed-off-by: F.N. Claessen <claessen@seita.nl> * Forward device_scheduler arguments to the highspy backend by name The dispatch to the direct HiGHS backend listed its keyword arguments by hand. That is a trap for the branches currently adding scheduling parameters: whoever adds the next one (coupling_groups in #2218, balance_groups in #2289) works on the Pyomo model further down the file, and a parameter missing from the dispatch list would not fail. It would simply never reach the backend, producing a schedule computed as if the constraint had never been requested -- and since this PR makes "highspy" the default solver, that would be silently wrong. Forward by name instead, mapping device_scheduler's signature onto the backend's. An argument the backend does not model raises NotImplementedError naming it, but only when the caller actually set it, so leaving a future parameter at its default stays free. The signature comparison is cached on the two function objects (~70 us once per process, 2 us per call after). Also record, in the highspy module docstring, that the deliberate omission of ems_flow_commitment_equalities stops being harmless once #2355 gives those rows bounds, and that #2380 routes unscoped flex-context commitments through grouped_commitment_equalities (which this backend does build). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen <claessen@seita.nl> * refactor: share the scheduler's solver-agnostic input handling The two backends build the same model in two representations, so the model construction is necessarily written twice. Everything around it was too: 199 lines were byte-identical between linear_optimization.py and highspy_optimization.py -- argument normalisation, stock groups and their validation, legacy commitment conversion, the sub-commitment split, device_group_lookup, the convex-curve check, the Big-Ms, band validation, the HiGHS option profile, and the cost/schedule assembly. None of it has a solver in it, and keeping it twice meant the two paths could drift apart on input handling, which the equivalence tests are not aimed at. Move it to a new scheduling_problem module: prepare_scheduling_problem() returns a SchedulingProblem that both backends unpack, plus solver_options() and the result-assembly helpers. Duplication between the backends drops from 199 to 40 lines, and those 40 are the shared signature and the call itself. Two deliberate changes while moving: - commodity_devices becomes a cached_property. It is a per-row scan that only the Pyomo ems_flow_commitment_equalities needs, so computing it eagerly would put a real cost on the fast path. Pyomo pays what it did before; the direct backend pays nothing until it needs it (see #2355). - initial_stock_of() casts its index to int, as the direct backend already did. The Pyomo version raised TypeError on the numpy float device indices a commitment's "device" column can carry. The empty-commitments case also stops raising on pd.concat([]), which previously made the Pyomo path crash where the direct path coped. Verified: 265 passed, 3 xfailed across the planning suite under all three solver parameters, plus the scheduling-job and API schedule tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen <claessen@seita.nl> * perf: build device_group_lookup from column arrays Sub-commitments are usually one row each (every time step tends to form its own commitment group), so this loop slices a fresh two-column DataFrame, runs dropna() on it and calls iterrows() once per time step. The pandas per-call overhead, not the work, dominated: profiling a 4-device x 192-step problem showed 192 dropna() calls accounting for ~50 ms of a ~135 ms prepare_scheduling_problem, against a model build measured in single-digit milliseconds. Read the two columns as arrays once and loop over them instead, replacing dropna() with an explicit missing-value check that also handles the collection-valued "device" entries pd.isna would answer element-wise. The loop itself goes from 49 ms to 1.0 ms (device+device_group) and 65 ms to 0.8 ms (device only) at 192 sub-commitments; prepare_scheduling_problem as a whole drops from 135 ms to 22 ms. Equivalence was checked against the previous implementation over device-only and grouped frames, NaN/None/pd.NA in either column, list/tuple/ndarray device entries, mixed group key types, stock-scoped and empty frames: same groups, same members, same insertion order. Reading the column array yields numpy scalars where iterrows() yielded Python floats on mixed-dtype frames; the two are interchangeable as set members and dict keys (equal hash and equality) and both survive the int() casts applied downstream. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen <claessen@seita.nl> * Build the EMS-level flow commitment rows in the direct HiGHS backend This backend skipped ems_flow_commitment_equalities because the Pyomo path built those rows without bounds, so they could never bind and HiGHS dropped them anyway. PR #2355 gives them the same one-sided bounds that grouped_commitment_equalities uses, so they bind now, and skipping them here would silently ignore an EMS-level commitment under this backend -- which this PR makes the default. The row is the grouped one with a different summation set, so rather than write it twice, the existing loop's body is extracted into _active_rows (the commitment's active time steps and bounds) and _add_commitment_rows (bind a commitment to the summed flow or stock of a set of devices). The EMS-level loop then sums over every device, or over the commitment's commodity's devices, and mirrors the Pyomo path in skipping commitments that name a device group -- those are already bound per group, and binding them twice over-constrains the problem. Two equivalence scenarios cover this, so it runs under both backends: ems_level_flow_commitment (names no device, binds all devices) and ems_level_commodity_commitment (binds only its commodity's devices, which also exercises the commodity_devices lookup). Both were checked to fail with the new rows disabled, so they cannot pass vacuously. Also drops the now-inaccurate deviation note from the module docstring. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen <claessen@seita.nl> * review: name the solver-results shim after Pyomo's own class "Stanza" was an unhelpful coinage. The attribute this shim stands in for holds a pyomo.opt.results.solver.SolverInformation, so name it _SolverInformation and say where the name comes from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen <claessen@seita.nl> * style: reflow docstrings and comments to break only after punctuation CLAUDE.md asks that docstrings and comments break lines only after punctuation, never mid-phrase, with max-line-length 160 and E501 ignored, so that review comments and text search stay stable. Copilot flagged nine places in this PR where I had not followed it, and it was right. Reflowed the docstrings and comments this PR adds, plus the ones it moved into the new scheduling_problem module. Doing that here rather than in the refactor commit keeps that commit verifiable as a verbatim move. Text only. No behaviour, names or logic changed; 290 passed, 3 xfailed across the planning suite under all three solvers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen <claessen@seita.nl> --------- Signed-off-by: F.N. Claessen <claessen@seita.nl> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Description
Fixes #2379: a regular (unscoped) flex-context commitment binds each device individually against its baseline, instead of the aggregate flow of all its commodity's devices. Regression from #1946 (multi-commodity), where
convert_to_commitmentswent from one EMS-levelFlowCommitment(**spec)(device=None, aggregate) to afor d in enumerate(flex_model): device=dloop (per-device).Fix
convert_to_commitments, bind oneFlowCommitmentper commitment over all of its commodity's devices:device=<the commodity's device indices>,device_group=<commodity>— mirroring the internal"<commodity> net energy"commitment.self.device_inventory.commodity_to_devices(canonical, and including the commodity's inflexible devices), instead of re-enumerating the rawflex_modellist — the positional-index reuse was a second latent bug.flex_modelparameter ofconvert_to_commitments.Tests
device_inventoryand assert the aggregate device set (they previously used one device per commodity, which couldn't distinguish per-device from aggregate).test_unscoped_commitment_binds_commodity_aggregate: two 8 MW heaters under a 10 MW baseline reach a combined 10 MW — a level neither could carry alone, and impossible under per-device binding.Changelog
Appended this PR to the existing multi-commodity changelog entry (the one that introduced the regression), rather than adding a new entry.
Relation to #2355 / #2326
Distinct bug. #2326/#2355 is about the solver's
ems_flow_commitment_equalitiesreturning an unbounded(None, expr, None)for EMS-level (device=None) commitments — affecting directdevice_schedulercallers. This PR fixes the StorageScheduler's per-device conversion, and its commitments usedevice=<list>(thegrouped_commitment_equalitiespath), so they don't go through thedevice=Nonecode #2355 fixes. The two are complementary. Heads-up: both touchtest_commitments.py, so whichever merges second will need a small rebase there (#2355 is currently CONFLICTING anyway).How to test
pytest flexmeasures/data/models/planning/tests/test_commitments.py🤖 Generated with Claude Code