Per-type service models with merged sparse/dense reserve containers - #206
Draft
rodrigomha wants to merge 57 commits into
Draft
Per-type service models with merged sparse/dense reserve containers#206rodrigomha wants to merge 57 commits into
rodrigomha wants to merge 57 commits into
Conversation
Convert POM to the per-type ServiceModel API: one model per service type (keyed by Symbol(D)), no per-service-name models. The nested per-service contributing_devices_map is filled once at finalize, and construct_service! runs once per type, iterating each service and reading its contributing devices from the map. - problem_template.jl: drop per-name set_service_model! and the aggregated expansion; rewrite _populate_contributing_devices! for the nested map; get_model keys services by Symbol(T). - services_constructor.jl: remove grouping; per-type single-model construct_service! for every reserve formulation, group reserve, and transmission interface (iterating get_available_components). - add_to_expression.jl / hydro / storage / hybrid: drive the reserve and interface folds per service; source the service name from the service. - add_expressions.jl: split the reserve-expression method so ProductionCostExpression keeps its per-service meta. - Tests migrated to ServiceModel(T, F); same-type duplicates collapsed.
Flip the service-indexed reserve containers (RequirementConstraint, ReserveRequirementSlack, and the ConstantReserveGroup RequirementConstraint) from sparse to dense. Each dense container is created once per service type in construct_service! over the full service-name axis, then filled per service. Device-indexed containers stay sparse.
…ype service model printing
…s with interface migration
_services_with_contributors tested emptiness via get_contributing_devices(model, name), which allocates a fully flattened device Vector per service. Switch to get_contributing_devices_map, which returns the inner dict without flattening. The map is never populated with empty inner dicts or empty device vectors (_add_contributing_device_by_type! only creates an entry when it pushes a device), so isempty on the map matches isempty on the flattened vector exactly.
The service-refactor planning docs under .claude/plans/ are working notes, not repo artifacts. Untrack them and gitignore the directory so they stay on disk locally but are never pushed.
Mark the comments that exist purely to narrate the before/after of the service container change (merged/sparse layout, empty meta) with a greppable DELETE-AFTER-REVIEW tag. These help reviewers understand why the change was made; grep the tag to delete them once the PR is approved so the code stops referring to how it worked before.
The 3-arg get!(dict, key, default) builds its default eagerly on every call, so _add_contributing_device_by_type! allocated a throwaway Dict and Vector even on the hit path where the service/type entry already exists. Switch to the lazy get!(f, dict, key) form so the fresh collections are built only on a miss. These are inserted and then mutated, so they cannot share IOM's read-only empty-map const.
Under the per-type ServiceModel, _populate_contributing_devices! populates every service of the type into one nested map and had no error path, so a reserve with no usable provider was silently dropped - an unmet requirement that would force slacks or make the model infeasible. Restore a loud error naming the offending service, scoped to S <: PSY.Reserve so group reserves, transmission interfaces, and AGC (which draw on services or branches, not provider devices) stay exempt - this also unblocks the old ConstantReserveGroup path that the blanket check on main used to break. get_available_components already limits the loop to available services, and _add_contributing_device_by_type! records only available devices (PSY's get_contributing_device_mapping returns unavailable ones too), so the check covers both a service with no devices assigned and a service whose devices are all unavailable. Add regression tests for both.
…ebase The rebase auto-merged a DCP-specific InterfaceTotalFlow add_to_expression! method that main added; it still used the pre-refactor get_service_name(model) and the flat get_contributing_devices_map(model). Under the per-type ServiceModel both are gone: read the service name from the service argument (PSY.get_name(service)) and the per-service nested map (get_contributing_devices_map(model, service_name)), matching the sibling interface methods. Textually clean merge, semantically broken - only surfaced at build time.
jd/converter-dc-current-base was merged into psy6, so move the PowerSystems source from that feature branch to psy6 in both the package and test environments. On the aligned psy6 stack (PSY/PNM/PF/PSB on psy6, IS on IS4 tip) the full suite is green, including the converter/HVDC tests whose set_max_dc_current unit error was resolved by the merge.
Bucket A (local function barriers; build-time paths, behavior-preserving): - reserves.jl ReservePowerConstraint: move the per-device body into a barrier so var_r/varstatus/cons container reads are statically dispatched instead of dynamic per device x timestep. - reserve_group.jl GroupReserve requirement sum: route the per-service scan through a barrier so add_to_expression! is statically dispatched, not per-container-entry dynamic dispatch inside the nested loop. - reserves.jl: fix a latent UndefVarError in a @debug (used undefined name; now PSY.get_name(d)). Bucket B (deferred; TODOs added so they are not forgotten): - services_constructor.jl: per-type-group barrier over contributing_devices_map to avoid the flattening element-type widening (POM twin of the deferred IOM per-type-group barrier). - problem_template.jl: abstract-eltype push into DeviceModel.services and the contributing_devices_map value vector (rooted in IOM struct fields, Comment A). - reserves.jl: O(entries) merged-container rescan in add_reserves_proportional_cost!. Full suite green (105421). See .claude/plans/service-refactor-stability.md.
Adds a build+solve test for the ConstantReserveGroup / GroupReserve path, which exercises reserve_group.jl add_constraints! and the _accumulate_group_reserve! function barrier (A3). The group contains only Reserve1, so the test asserts every Reserve1 device reserve variable enters the group RequirementConstraint with coefficient 1 and every Reserve11 variable with coefficient 0, verifying the per-service key filtering and the full-slice summation. This path was previously untestable: the old _populate_contributing_devices! errored on any ConstantReserveGroup. It is now reachable because that error is scoped to PSY.Reserve. Removes the corresponding NOT-PORTED note.
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors reserve service modeling to be per-service-type (like DeviceModel) and updates reserve-related containers to be merged per type (sparse (service, device, time) for device-indexed entries; dense (service, time) for requirement-side entries), removing the prior per-service-name meta approach.
Changes:
- Reworked service construction to run once per service type, iterating each service’s name to fill slices of merged containers (reserves, groups, transmission interfaces).
- Updated reserve slacks/requirements/variables to use merged dense/sparse containers and adjusted expressions/objective wiring accordingly.
- Updated tests, docs, printing, and dependency pins to align with the new per-type
ServiceModelAPI and container shapes.
Reviewed changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_storage_device_models.jl | Updates reserve ServiceModel(...) calls to the new per-type (no per-name) API. |
| test/test_services_constructor.jl | Adds/updates regression tests for merged reserve containers, per-type slacks, duals, and new populate-time errors. |
| test/test_model_decision.jl | Updates output assertions for merged reserve variable containers and flattened WIDE results. |
| test/test_device_hydro_constructors.jl | Adjusts hydro constructor tests to use per-type reserve service models. |
| test/test_device_hybrid_constructors.jl | Updates hybrid template reserve modeling to register each reserve type once. |
| test/Project.toml | Pins InfrastructureOptimizationModels to the refactor branch and updates PowerSystems rev. |
| src/utils/print.jl | Simplifies service-model printing to reflect type-keyed service models (no per-name rows). |
| src/static_injector_models/hydro_generation.jl | Updates reserve expression building to index merged (service, device, time) reserve variables. |
| src/services_models/services_constructor.jl | Refactors service construction flow for per-type models; builds shared containers once and fills per-service slices. |
| src/services_models/service_slacks.jl | Renames/rewrites reserve slack creation for dense per-type (service, time) container. |
| src/services_models/reserves.jl | Migrates reserve constraints/variables/costing to merged containers and introduces per-service summation helper. |
| src/services_models/reserve_group.jl | Updates group reserve requirement to sum contributing services’ slices from merged reserve containers. |
| src/services_models/agc.jl | Removes now-obsolete aggregated_service_model default attribute. |
| src/initial_conditions/initialization.jl | Updates IC template service-model propagation to use per-type set_service_model!. |
| src/hybrid_system_models/hybrid_systems.jl | Updates hybrid reserve expression/constraints to index merged reserve variables by (service, device, time). |
| src/energy_storage_models/storage_models.jl | Updates storage reserve expression building to index merged reserve variables by (service, device, time). |
| src/core/problem_template.jl | Reworks template service-model storage to be keyed by type only; implements per-service nested contributing-device maps and new reserve erroring behavior. |
| src/core/definitions.jl | Removes NO_SERVICE_NAME_PROVIDED constant tied to the old per-name service model API. |
| src/common_models/market_bid_overrides.jl | Updates delta-PWL linkage callsite to match merged ServiceRequirementVariable behavior (no per-service meta). |
| src/common_models/add_to_expression.jl | Updates reserve add-to-expression paths to accept the concrete service and index merged containers; updates interface contributing-device map access. |
| src/common_models/add_parameters.jl | Adds per-type service time-series parameter container construction (dense, keyed by service name). |
| src/common_models/add_expressions.jl | Removes per-service meta for shared reserve expressions; adds temporary per-service meta path for ORDC cost expressions (deferred migration). |
| Project.toml | Pins InfrastructureOptimizationModels and PowerSystems to branches needed for this refactor. |
| docs/src/reference/optimization_container_axes.md | Documents merged per-type service containers and their axes/indexing patterns. |
| .gitignore | Ignores local .claude/* artifacts added for reviewer context/plans. |
Comments suppressed due to low confidence (1)
test/test_services_constructor.jl:213
@test_throwsdoes not accept a string message matcher. To verify the constructor errors on the all-devices-unavailable path and includes the expected text, capture the exception and assert on its message.
@test_throws "no available contributing devices" DecisionModel(
template,
sys;
optimizer = HiGHS_optimizer,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per odow's suggestion in #202: a type is itself a zero-arg constructor, so get!(Dict{...}, dict, key) / get!(Vector{T}, dict, key) give the lazy (miss-only) default without an anonymous function. Same semantics as get!(() -> ..., ...), less ceremony. Covers both defaults in _add_contributing_device_by_type!.
Phase A moved the TransmissionInterface flow-limit params to the merged vector path and C1 uses the ObjectiveFunctionParameter path, so the single-service TimeSeriesParameter add_parameters!/_add_parameters! (service::U, ServiceModel) have no remaining callers in src. Remove both. Every add_parameters! call site now passes a vector.
A device can offer a PWL price/quantity curve for providing a reserve, stored via the PSY set_service_bid! path (a PiecewiseStepData time series named after the service, on a device with an OfferCurveCost; membership in MarketBidCost.ancillary_service_offers). This is the (service, device, segment, time) 4D cost structure - one PWL offer per (device, service) per hour, on top of the already-3D (service, device, time) reserve award. add_device_reserve_offers! builds such a fixture; the tests pin the data model (get_services_bid returns a per-device offer curve) and that the fixture builds and solves. Today these offers are NOT consumed - the reserve is priced flat (DEFAULT_RESERVE_COST) and no reserve-offer cost term is built; that consumer is the next step.
Consume the per-device reserve OFFER curves (set via PSY `set_service_bid!`,
retrieved with `get_services_bid`) instead of pricing every reserve award at the
flat `DEFAULT_RESERVE_COST`. This is the (service, device, segment, time) 4D cost
structure sitting on top of the 3D `(service, device, time)` reserve award.
- `PiecewiseLinearBlockReserveOffer` (SparseVariableType) + `ReserveOfferLinkingConstraint`.
- `add_reserve_offer_costs!` builds the delta/block-offer formulation per offering
device: segment vars `δ_k >= 0`, `δ_k <= P_{k+1}-P_k`, `Σ_k δ_k == award`,
`objective += Σ_k slope_k · δ_k · dt`. Reuses the device-offer PWL machinery
(`_get_raw_pwl_data`, `get_piecewise_curve_per_system_unit`,
`get_pwl_cost_expression_delta`). Returns the priced device names.
- `add_to_objective_function!` prices offering devices here, then the flat-cost pass
skips them via a new `skip_devices` kwarg on `add_reserves_proportional_cost!`.
The 4D block var is created through the axes+`sparse=true` container path so the key
type is `(String,String,Int,Int)`; the SparseVariableType default is hardcoded 3-tuple.
Test asserts the block var, the award-linking constraint (±1 coefficients), and the
offer-slope objective coefficient (segment-2/segment-1 ratio = 1.5).
…atch predicate
Addresses the four review items on the C2 consumer:
1. Proper 4D container. Consume a new IOM trait `sparse_variable_key_type` (default 3D
`(device, segment, time)`) overridden for `PiecewiseLinearBlockReserveOffer` to the 4D
`(service, device, segment, time)` key, so the SparseVariableType auto-path builds the
right container. Replaces the `1:1` axis key-type-carrier workaround.
2. Magnitude assertion. The test now checks the objective coefficient equals
`slope * base_p * dt` (an independent reference), not just the 1.5x segment ratio -
this pins the natural->system-base conversion, catching a silent wrong-magnitude cost.
3. Dispatch instead of `isa`. `_has_reserve_offer` dispatches on the operation-cost type
(`OfferCurveCost` offers; `OperationalCost` fallback returns false).
Also restores the `names` binding that had silently resolved to `Base.names`, which had
widened the linking-constraint key to `Tuple{String, Any, Int}`; now `Tuple{String, String, Int}`.
Reserve-offer PWL block vars remain unexported in results (verified export is structurally
correct - sum(delta)*base_p == award, cheapest segment fills first - but delta exports in pu,
mismatched against the MW-natural award, so it stays off).
Depends on an IOM change adding `sparse_variable_key_type` (rh/dev_service_refactor).
Add a short explanation to the 'Attach a per-device reserve offer' section covering the two persistent effects of set_service_bid! (attach the named offer time series, register the service in ancillary_service_offers) plus its validation, and why they let the model price the reserve award by the per-device curve.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 33 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
docs/src/reference/optimization_container_axes.md:112
- This paragraph says transmission-interface containers and ORDC (
StepwiseCostReserve) PWL cost parameters “are not migrated yet and still use meta”, but this PR migrates both: interface slacks/flow-limit params are merged per type (empty meta), and ORDC slope/breakpoint parameters are merged per service type (empty meta). Update this text to match the implemented container shapes.
above. Storage/hybrid reserve sub-containers, the transmission-interface containers, and the
ORDC (`StepwiseCostReserve`) piecewise cost parameters are **not** migrated yet and still use
`meta`; interface *construction* is per-type (iterates interfaces, reads the per-service map)
but its containers stay `meta`-keyed.
docs/src/reference/optimization_container_axes.md:44
- This table row is now inaccurate:
InterfaceFlowSlackUp/Downare no longer built via themeta=overload nor limited to a singleton[interface_name]axis. Insrc/services_models/service_slacks.jl,transmission_interface_slacks!builds one dense 2D container per slack type over all interface names (empty meta). This row should be removed from the “synthetic singleton axis” section (or rewritten and moved).
This issue also appears on line 109 of the same file.
| `InterfaceFlowSlackUp` / `InterfaceFlowSlackDown` | `services_models/service_slacks.jl` | `[interface_name]` (1st axis) | Built via the `meta`-keyword overload (`add_variable_container!(container, T, U, [interface_name], time_steps; meta=interface_name)`) so `meta` disambiguates same-type interfaces while the axis stays real; indexed `[interface_name, t]`. (Transmission-interface migration to merged containers is deferred.) |
Both reserve add_expressions! methods described the container in terms of the old
per-service meta ('empty meta', 'no per-service meta'). Under per-type service models
the default meta is the norm, so the reference is stale - trim to plain functional
descriptions of the merged containers.
…rences)
Make the service-type UnionAll comments version-neutral (drop the 'PSY6' tag and the
'unit-system type' implementation detail, keep the ReserveDemandCurve{ReserveUp} example),
and trim the historical 'bug that previously made ConstantReserveGroup unbuildable'
parenthetical to the forward-looking rationale.
Remove the 'See .claude/plans/service-refactor-stability.md (deferred B4)' pointer; keep the actionable efficiency note (keyed slice instead of full .data scan).
Extend the GroupReserve tests beyond the single-service requirement sum: - multi-service group sums both contributing services' slices - ReserveDown-direction group builds and solves - positive group requirement is enforced in the solution - build fails when a contributing service is not modeled (check_activeservice_variables) - group is constructed last regardless of template insertion order Closes #151.
…r (group, t) Replace _accumulate_group_reserve! (which scanned the whole merged (service, device, time) container per (group, t)) with _group_member_variables, which buckets the members' variables by (service_name, t) in a single pass - deduped by service type since same-type members share one merged container. The constraint loop now does keyed lookups. Resolves the efficiency TODO; O(entries x members x timesteps) -> O(entries + members x timesteps). Behavior unchanged (GroupReserve tests still pass).
Drop '(empty meta)' from two merged-container comments and remove the 'deferred B4' label + .claude/plans pointer from the flat-cost efficiency TODO, keeping the actionable note. Comments only.
…ull scan add_reserves_proportional_cost! now takes the service's contributing device names and indexes reserve_variable[(service, name, t)] directly instead of scanning the whole merged container per service; add_to_objective_function! passes the names. Every (service, device, t) key is guaranteed present (created from the same get_contributing_devices list), so indexing is safe. Resolves the last services-efficiency TODO; O(entries x #services) -> O(entries). Behavior unchanged (reserve pricing / offer-skip tests still pass).
Drop the .claude/hooks/, .claude/settings.json, .claude/plans/ ignore lines added to the repo .gitignore; those are per-developer local config and belong in a local git exclude, not the shared repo. Restores .gitignore to match main.
…+ test comments - services_constructor.jl, transmission_interface.jl: drop '(empty meta)' and the last DELETE-AFTER-REVIEW reviewer-context blocks, keeping functional descriptions. - test_model_decision.jl, test_services_constructor.jl: remove DELETE-AFTER-REVIEW markers and empty-meta references; de-version two 'psy6 data model' comments to 'current PowerSystems'. Comments only. Completes the DELETE-AFTER-REVIEW sweep (none remain repo-wide).
StepwiseCostReserve now calls add_reserve_offer_costs! alongside its elastic ORDC/ASDC demand-curve pricing, so a per-product demand curve and per-resource ancillary-service offer costs co-optimize in one objective. No flat DEFAULT_RESERVE_COST fallback is added, so pure-ORDC services with no device offers are unchanged. Tests: offer pricing (containers + objective coefficients), per-hour participation (non-offer hours capped by the offer curve), no-offer ORDC regression, and offer merit order (cheapest offer clears, priciest does not even at larger capacity). Solved-value reads use the read_variable results API.
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
Refactors reserve/ancillary-service service modeling from the service-name-as-
metapattern to per-typeServiceModels with merged containers, and builds on that to add per-device service bids. OneServiceModelnow covers every service of a type (likeDeviceModel); each service fills its own slice of a shared container:ActivePowerReserveVariable, participation/power constraints): one merged sparse 3D container keyed(service_name, device_name, time).RequirementConstraint,ServiceRequirementVariable, slacks): one dense 2D container keyed(service_name, time).This removes the
aggregated_service_model/NO_SERVICE_NAME_PROVIDEDmachinery and the per-service-name model API, replacing it with type-keyedset_service_model!mirroringset_device_model!.This PR consolidates the full effort (previously stacked as #207 Phase A+B and #210 Phase C; both are now folded in here).
What's included, by phase
Core — per-type service models
ServiceModelAPI and per-type construction; sparse service-indexed reserve variables; dense service-indexed requirement containers.aggregated_service_model/NO_SERVICE_NAME_PROVIDED; fixed per-type service-model printing.Phase A — TransmissionInterface
Phase B — ORDC cost expression
ProductionCostExpressioninto a merged dense(service, time)container.Phase C1 — ORDC cost parameters
TimeSeriesParameters into one container per service type (covariantVector{<:ReserveDemandTimeSeriesCurve}axis + batch-wide tranche padding).TimeSeriesParameteradd_parameters!/_add_parameters!path.Phase C2 — per-device service bids (reserve offers)
set_service_bid!/get_services_bid); its reserve award is priced by that curve instead of the flatDEFAULT_RESERVE_COST.PiecewiseLinearBlockReserveOffer(SparseVariableType, 4D(service, device, segment, time)) +ReserveOfferLinkingConstraint;add_reserve_offer_costs!builds the delta/block-offer formulation (δ_k ≥ 0,δ_k ≤ P_{k+1}-P_k,Σ δ_k = award,objective += Σ slope_k·δ_k·Δt). Devices without an offer keep the flat cost via askip_devicespass.sparse_variable_key_typetrait (see Complete the device time-varying market-bid offer-curve path #141).Cross-cutting
ConstantReserveGroup. This unblocksConstantReserveGroupconstruction, which was previously unbuildable; GroupReserve now has end-to-end test coverage (single/multi-service requirement sums, both reserve directions, requirement enforcement, the unmodeled-member error path, and construction ordering). Closes Testing for GroupReserve #151.UndefVarErrorin a@debug).get!cleanup: type-as-constructor defaults (get!(Vector{T}, …)/get!(Dict{…}, …)) instead of anonymous closures in the contributing-devices map builder (per odow's suggestion in Use lazy get!(f, dict, key) to avoid eager-default allocations on the hit path #202). The remaining eagerget!sites in other subsystems are handled separately in Lazy type-as-constructor get! defaults (closes #202) #215.Docs
docs/src/tutorials/service_bids.jl) walking through building a system, attaching per-device reserve offers, solving, and reading results.EXECUTE = TRUEopt-in (non-executed tutorials render as plainjuliablocks, so a tutorial needing solvers/data the docs env lacks still builds).Dependencies
rh/dev_service_refactor): theOptimizationContainer/store/ServiceModelchanges and the newsparse_variable_key_typetrait live in InfrastructureOptimizationModels.[sources]here points at that branch; revert to a registered/mainrev once Complete the device time-varying market-bid offer-curve path #141 merges.psy6, on the current IS4 InfrastructureSystems tip.Testing
Full suite green: 31244 / 31244, no failures.
The transformer control formulations and the PowerFlows-dependent suites are currently disabled in
test/runtests.jl/test/includes.jl(psy6: disabled pending transformer refactor—PowerFlows@psy6does not precompile:ThreeWindingTransformerWinding→ThreeWindingTransformerCircuit). That gating is inherited frommainand is why the suite total is lower than pre-rebase; it is not a regression in this PR. Those suites re-enable once the upstream transformer refactor lands.Reviewer notes
DELETE-AFTER-REVIEWexplain the before/after of the container change; remove once approved (grep -rn "DELETE-AFTER-REVIEW").TODO(services …): the per-(device-type) function barrier overcontributing_devices_mapand the abstract flattened-Vectorroots (in IOM structs) for type stability; the merged-container full scans in the flat-cost/group-reserve paths (replace with keyed slices); per-service keying pending the reserve-feedforward port.mainis 5 commits ahead (CI/docs only — codecov/actions/Sienna.md); the PR isMERGEABLEwith no conflicts. A final rebase onto latestmainis needed at merge time (and will re-enable the transformer/PF suites only once that refactor lands).🤖 Generated with Claude Code