Add hydro turbine linear commitment formulation#84
Add hydro turbine linear commitment formulation#84acostarelli wants to merge 22 commits intomainfrom
Conversation
…e-dispatch' into ac/waterlinear-commit
…r hydro constructors
luke-kiernan
left a comment
luke-kiernan
left a comment
There was a problem hiding this comment.
Based on the fact that you're passing types into get_multiplier_value, I'm inferring that this code wasn't actually run. Will review again once tests work
Co-authored-by: Luke Kiernan <86331877+luke-kiernan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new hydro turbine formulation that models linear water-to-power conversion with a binary commitment (on/off) decision, aligning this package with the upstream HydroPowerSimulations.jl port.
Changes:
- Introduces
HydroTurbineWaterLinearCommitmentformulation and exports it. - Adds device construction logic (variables/constraints/objective wiring) to support the new formulation.
- Extends hydro cost handling for time-variant proportional terms and adds a regression test covering reservoir budget + turbine commitment.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/core/formulations.jl |
Defines the new HydroTurbineWaterLinearCommitment formulation type. |
src/PowerOperationsModels.jl |
Exports the new formulation so it’s usable by templates. |
src/static_injector_models/hydrogeneration_constructor.jl |
Adds construct_device! implementations for the new commitment formulation. |
src/static_injector_models/hydro_generation.jl |
Extends variable/constraint support for the new formulation and adds “variant” proportional-cost plumbing. |
test/test_device_hydro_constructors.jl |
Adds a solve test for HydroWaterModelReservoir with budget + the new commitment turbine model. |
Project.toml |
Adds an InfrastructureSystems [sources] override and modifies compat entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lin_cost = _add_proportional_term_helper( | ||
| container, T(), component, linear_term, time_period) |
There was a problem hiding this comment.
_add_proportional_term_variant! calls _add_proportional_term_helper, but that helper isn’t defined anywhere in the repo. This will throw a UndefVarError at runtime whenever a time-variant proportional cost term is encountered (e.g., MarketBidCost on-costs). Consider inlining the same variable lookup logic used in _add_proportional_term! (but adding to the variant objective expression) or defining the missing helper.
| lin_cost = _add_proportional_term_helper( | |
| container, T(), component, linear_term, time_period) | |
| component_name = PSY.get_name(component) | |
| variable = get_variable(container, T, U)[component_name, time_period] | |
| lin_cost = variable * linear_term |
| end | ||
| add_to_expression!(container, ProductionCostExpression, exp, d, t) | ||
| end | ||
| cost_term = proportional_cost(op_cost_data, U, d, V) |
There was a problem hiding this comment.
@luke-kiernan Is this proper use of the new IOM code? Or would it be better to just have the one add_proportional_cost function? I don't know how much performance is lost to the "maybe variant" part.
There was a problem hiding this comment.
Your definition here is non-time-varying, so I suspect it's redundant: the default add_proportional_cost! in IOM should handle it just fine.
There was a problem hiding this comment.
There's bigger structural issues here: this file is still basically a copy-paste from PSI. In the initial PSI-into-POM/IOM split, I deferred refactoring the hydro code to "later," perhaps due to the separate HPS package. I never returned to that task. I've opened #96 for this and will tackle it ASAP (today and tomorrow).
Until that's finished, imo it doesn't make sense to merge this. Core issues first then add new features.
Aside: clearly we need better test coverage. This file calls functions that don't exist.
…/waterlinear-commit
|
Performance Results
|
| return | ||
| end | ||
|
|
||
| _maybe_add_on_variables!( |
There was a problem hiding this comment.
@luke-kiernan What do you think of this pattern?
| """ | ||
| struct HydroPumpEnergyCommitment <: AbstractHydroPumpFormulation end | ||
|
|
||
| """ |
There was a problem hiding this comment.
@luke-kiernan and this? For simplifying unions
Port of Sienna-Platform/HydroPowerSimulations.jl#126