Port hybrid systems#104
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
JuliaFormatter
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybrid_systems.jl
Lines 819 to 820 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybrid_systems.jl
Lines 1380 to 1390 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybrid_systems.jl
Lines 1442 to 1443 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybrid_systems.jl
Lines 1483 to 1484 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybrid_systems.jl
Lines 1551 to 1553 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 132 to 134 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 183 to 184 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 191 to 192 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 211 to 214 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 216 to 217 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 223 to 224 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 252 to 253 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 296 to 297 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 303 to 304 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 321 to 324 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 326 to 327 in 4ad672f
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 332 to 333 in 4ad672f
|
Performance Results
|
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
JuliaFormatter
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 143 to 144 in e0d67dc
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 193 to 194 in e0d67dc
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 201 to 202 in e0d67dc
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 221 to 224 in e0d67dc
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 226 to 227 in e0d67dc
[JuliaFormatter] reported by reviewdog 🐶
PowerOperationsModels.jl/src/hybrid_system_models/hybridsystem_constructor.jl
Lines 233 to 234 in e0d67dc
There was a problem hiding this comment.
We need to add a couple of extra tests.
First, we need to test different attributes, that is using reservation = false, and energy target = true. Also, what happened with storage_reservation and regularization attributes?
Also, it would be good to test that the hybrid system builds when there is no thermal, or no storage, or no renewable.
Finally, it would be good to test when the hybrid does not participate in reserves.
Overall, the PR looks good. Once we have the tests, I will QA/QC the model for the different tests to ensure that the constraints are looking good.
| # Hybrid System Parameters | ||
| ################################################################################# | ||
|
|
||
| "Time-series parameter for the maximum active power available from a hybrid system's |
There was a problem hiding this comment.
@jd-lara This is a change from how things are currently defined in HSS with the time series attached to the hybrid, not the subcomponent, but I guess that works and is simpler for this case?
kdayday
left a comment
kdayday
left a comment
There was a problem hiding this comment.
Following Rodrigo's lead on the testing, but in addition, can you port over the updated equivalent of what's in the HybridDispatchWithReserves docstring on the HSS PR and have claude add hyperlinks to exported types and functions referenced in the docstring? It looks like the POM docs are way out of date, but would like to update as we go.
There was a problem hiding this comment.
Pull request overview
This PR ports/introduces PSY.HybridSystem support into PowerOperationsModels.jl by adding a new HybridDispatchWithReserves device formulation, its constructor plumbing, and a basic end-to-end test that builds/solves a hybrid co-optimization on RTS-GMLC.
Changes:
- Adds hybrid formulation types, variables/parameters/expressions/constraints, and objective plumbing for
HybridDispatchWithReserves. - Adds a two-stage
construct_device!implementation for HybridSystems (argument + model stages) and hooks it into the main module includes/exports. - Adds test utilities and a new test that builds and solves a model containing a HybridSystem with reserves.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/PowerOperationsModels.jl |
Includes hybrid model files and exports the new hybrid-related types. |
src/hybrid_system_models/hybrid_systems.jl |
Implements hybrid variable bounds, reserve aggregation, constraints, and objective-cost plumbing. |
src/hybrid_system_models/hybridsystem_constructor.jl |
Adds the two-stage construct_device! pipeline for HybridDispatchWithReserves. |
src/core/variables.jl |
Introduces HybridSystem-specific variable types. |
src/core/parameters.jl |
Introduces HybridSystem time-series parameter types and unit-conversion behavior. |
src/core/expressions.jl |
Adds HybridSystem reserve aggregation/served-reserve expression types. |
src/core/constraints.jl |
Adds HybridSystem constraint type definitions. |
src/core/formulations.jl |
Adds hybrid formulation types and user-facing documentation. |
test/test_utils/hybrid_test_utils.jl |
Adds fixtures for building a test HybridSystem in RTS-GMLC. |
test/test_device_hybrid_constructors.jl |
Adds an integration-style test that builds and solves a hybrid dispatch-with-reserves model. |
test/includes.jl |
Wires hybrid test utilities into the test suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ularization attributes; enable formatting; use jump utils; port docstrings;
Collapse paired Charge/Discharge, In/Out, Up/Down add_constraints! and
add_to_expression! methods into single bodies that dispatch on small
type-keyed trait stubs. Same JuMP shapes, same constraint meta strings,
same dispatch reachability. No public API change.
Hybrid (src/hybrid_system_models/hybrid_systems.jl):
- Charge/DischargeRegularizationConstraint
- HybridStorageStatus{Charge,Discharge}OnConstraint
- HybridStorage{Charging,Discharging}ReservePowerLimitConstraint
(folds in the _ch/_ds_reserve_up_dn_exprs helpers)
- HybridTotalReserve{Up,Down}Expression /
HybridServedReserve{Out,In}{Up,Down}Expression add_to_expression!
- ReserveAssignment/Deployment{Up,Down}{Charge,Discharge} ←
Hybrid{Charging,Discharging}ReserveVariable add_to_expression!
- HybridStatus{Out,In}OnConstraint
Storage (src/energy_storage_models/storage_models.jl):
- StorageRegularizationConstraint{Charge,Discharge}
Verified: 71/71 hybrid + storage tests pass. Net -376 lines.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace three custom hybrid constraint types and their hand-written
add_constraints! bodies with a single call to IOM's
`add_semicontinuous_range_constraints!`, paralleling how
`AbstractThermalUnitCommitment` handles the same range-with-on-variable
pattern at thermal_generation.jl:405-419.
Mechanism:
- Define `get_min_max_limits(::PSY.HybridSystem, ::ActivePowerVariableLimitsConstraint, ::AbstractHybridFormulation)`
to read `PSY.get_active_power_limits(PSY.get_thermal_unit(d))`. IOM's
helper picks up `OnVariable` keyed by `PSY.HybridSystem` automatically.
- For the with-reserves case, introduce two expression types subtyping
`RangeConstraint{UB,LB}Expressions`: `HybridThermalActivePowerWithReserve{UB,LB}`.
Argument-stage `add_expressions!` aggregates `p_th + Σ r_up` (UB) and
`p_th − Σ r_dn` (LB) into them, after which IOM's expression-typed
dispatch emits `min·on ≤ p_th − r_dn` and `p_th + r_up ≤ max·on` directly.
Removes:
- HybridThermalOnVariableUbConstraint, HybridThermalOnVariableLbConstraint,
HybridThermalReserveLimitConstraint (constraint types + exports)
- _thermal_reserve_up_expr / _thermal_reserve_down_expr helpers
- Three add_constraints! bodies (~190 lines)
Renewable cases stay hand-written for now: IOM's parameterized helper
filters by `IS.has_time_series(d, ts_type, ts_name)`, and PSY's
HybridSystem doesn't expose its inner RenewableDispatch's time series
through that accessor. A separate change would be required.
Verified: 50/50 hybrid tests pass.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This reverts commit 59c994b.
Per PR review (acostarelli): "domain" describes the value space of each variable (a discrete set or continuous interval) more accurately than "bounds", which suggests inequality-only. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address PR review feedback:
- "Don't use `isa`. Add a method to handle this, or restructure
existing dispatch." Eliminates every `isa(service, PSY.Reserve{...})`
and `service isa skip_kind` site in hybrid_systems.jl. The
`_excluded_reserve_kind` trait stub goes away — its information is
now encoded by union types in helper method signatures.
Five sites refactored, all sharing the same shape (per-direction
no-op methods + a fallback `::PSY.Service` work method):
- `add_to_expression!` for HybridTotalReserveExpression /
HybridServedReserveExpression → `_accumulate_reserve!`
- `add_to_expression!` for the eight Reserve*Balance{Up,Down}
{Charge,Discharge} expressions → `_balance_term!` plus a
`_deployment_factor` per-T trait that replaces the
`is_up`/`is_deployment` Booleans
- `_renewable_reserve_up/down_expr` → `_renewable_reserve_*_term!`
thunks sharing `_accumulate_renewable_reserve!`
- `_thermal_reserve_up/down_expr` → analogous restructure
- `add_constraints!` for ReserveCoverageConstraint{,EndOfPeriod}
→ `_init_coverage_container!` + `_emit_coverage_constraint!`;
the `(service isa PSY.Reserve) || continue` guard becomes the
`::PSY.Service` fallback no-op
Helper arguments use concrete types (OptimizationContainer, String,
Int, Float64, PSY.Storage, …) plus parametric `::Type{T}`/`::Type{U}`/
`d::V`/`::W` to reduce precompilation overhead.
- "Combine these if statements." Merges the two adjacent setup
ternaries (`r_ub, r_lb = if has_reserves …` and
`con_lb = if has_reserves …`) in
`add_constraints!(::Type{HybridStatus{Out,In}OnConstraint}, …)` into
a single 3-tuple ternary.
Test.detect_ambiguities returns 0; full suite passes (50/50).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…milies
Hybrid reserve variables, expressions, and constraints had ~16 paired Charge/Discharge,
Up/Down, Total/Served (a.k.a. Assignment/Deployment), and UB/LB singleton structs plus
~14 paired trait helpers that all differed only by which sibling they referenced.
Introduce marker singletons for ReserveSide, ReserveDirection, ReserveScale (UnscaledReserve
/ DeployedReserve), reuse IOM's BoundDirection (UpperBound/LowerBound), and reparametrize
the family roots:
- ReserveAggregationExpression{D,S,Sd} umbrella with two concrete struct families
(HybridPCCReserveExpression, StorageReserveBalanceExpression) covering all 16
historical reserve-expression singletons.
- HybridPCCReserveVariable{Sd}, HybridStorageSubcomponentReserveVariable{Sd},
HybridStorageSubcomponentPower{Sd}, RegularizationVariable{Sd}.
- HybridStatusOnConstraint{Sd}, HybridStorageStatusOnConstraint{Sd},
HybridStorageReservePowerLimitConstraint{Sd}, RegularizationConstraint{Sd},
HybridThermalOnVariableConstraint{B}.
All 34 historical concrete names are retained as const aliases so external imports,
`get_expression`/`get_variable`/`get_constraint` lookups, and module exports are
byte-compatible. Inside hybrid_systems.jl this lets:
- _accumulate_reserve! + _balance_term! collapse into one _add_reserve_term! family
(PCC boundary and storage subcomponent share the no-op skip and scale dispatch);
- thermal/renewable subcomponent accumulators (10 helpers) collapse into one
_subcomponent_reserve_term! / _subcomponent_reserve_expr family parametric on
the variable type;
- the UB/LB thermal-on-variable add_constraints! methods merge;
- ~14 paired trait helpers (storage / PCC / regularization) become parametric
single-method definitions;
- 5 file-local Union consts (_BalanceUpExpr, _BalanceDownExpr, _BalanceDeploymentExpr,
_HybridReserveUpExpr, _HybridReserveDownExpr) and _StorageCharge/DischargeSide
Union consts get deleted.
Additional cleanups:
- get_variable_multiplier hybrid signatures take ::Type{<:Formulation} (matches the
rest of POM); all W() instance call-sites become type-keyed.
- Three `if W <: ...` body-level subtype checks split into separate parametric
dispatched methods (HybridStorageBalanceConstraint, RegularizationConstraint,
HybridStatusOnConstraint).
- _init_coverage_container! uses lazy_container_addition! (idempotent).
- add_proportional_cost!(OnVariable, hybrids) hoists the variant/invariant
function-handle selection out of the per-t loop.
Net: -142 lines across 5 files. Full Pkg.test passes (13125 / 0 fail / 0 error / 1
pre-existing broken). Zero method ambiguities.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
|
||
| **Time Series Parameters:** | ||
|
|
||
| | Parameter | Default Time Series Name | |
There was a problem hiding this comment.
@acostarelli this documents how things are defined in the current HybridSystemSimulations. Looking at where HybridRenewableActivePowerTimeSeriesParameter is defined below, I'm actually not sure if in POM the time series is attached to the HybridSystem vs. the subcomponents, but I'm also not finding any other references to "RenewableDispatch__max_active_power" besides the comment, so I assume the time series are not fully implemented as they are in HSS?
|
Major concern here regarding parametric structs. Did we decide to go using Parametric types for defining constraints or expressions? I think it could be a good plan here for Hybrid and all the combinations rather than having multiple structs. What are your thoughts here @jd-lara ? For example we have expressions with 3 parametric types, e.g. ReserveAggregationExpression. My main comment is that we have a lot of const for handling the different parametric types. @acostarelli What is the reason for having multiple const rather than multiple structs? Does using parametric types makes it better to reuse code with multiple dispatch? I'm not following that much this, but could be? In any case @acostarelli if we decide to go in this direction, we should change the generic Up and Down structs for something better defined, e.g. UpDirection and DownDirection, rather than simply Up and Down I will use TotalReserve rather than UnscaledReserve. You also have the const using Total for the Unscaled type. Also, I'm not following exactly what ReserveSide with ChargeSide and DischargeSide. I think maybe eliminating this ReserveSide thing, and only leave two parametric types could be simpler than having so many consts. Overall the PR looks quite good, so I would just focus on deciding about the parametric types, but the tests looks quite comprehensive. |
| # singletons across the codebase: a single parametric struct + const aliases replaces | ||
| # every (Charge/Discharge), (Up/Down), (Unscaled/Deployed), (UB/LB) sibling pair. | ||
|
|
||
| abstract type ReserveDirection end |
| # Constraint UB/LB axis: reuse IOM's `BoundDirection` / `UpperBound` / `LowerBound` | ||
| # (defined in InfrastructureOptimizationModels/common_models/constraint_helpers.jl). | ||
| # A local alias keeps the abstract name discoverable through POM exports. | ||
| const ConstraintBound = InfrastructureOptimizationModels.BoundDirection |
IOM #104 ("Redistribute operation models") de-power-ified IOM, leaving the power-flavoured problem taxonomy to POM. This renames POM's problem-type hierarchy and operations template to the explicit de-power-ified convention: PowerOperationModel -> AbstractPowerOperationProblem DecisionProblem -> AbstractPowerDecisionProblem EmulationProblem -> AbstractPowerEmulationProblem DefaultDecisionProblem -> DefaultPowerDecisionProblem DefaultEmulationProblem -> DefaultPowerEmulationProblem GenericOpProblem -> GenericPowerDecisionProblem GenericEmulationProblem -> GenericPowerEmulationProblem OperationsProblemTemplate -> PowerOperationsProblemTemplate Also: - Add the bare-system ArgumentError constructor stub for DefaultPowerDecisionProblem (a template is required). - Remove dead Simulation* test scaffolding that referenced IOM types removed by #104 (test_utils/run_simulation.jl and two unused mock helpers); mirrors IOM #104's own deletion of run_simulation.jl. POM precompiles and loads cleanly against the merged IOM main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Thanks for opening a PR to PowerOperationsModels.jl, please take note of the following when making a PR:
Check the contributor guidelines