Skip to content

Add attribute to select approximation method#122

Open
acostarelli wants to merge 5 commits into
mainfrom
ac/hydro-milp-rename
Open

Add attribute to select approximation method#122
acostarelli wants to merge 5 commits into
mainfrom
ac/hydro-milp-rename

Conversation

@acostarelli
Copy link
Copy Markdown
Member

@acostarelli acostarelli commented May 13, 2026

An alternative could be to have a different formulation struct for each approximation method, sort-of mirroring the configs already in IOM e.g. like HydroBin2 HydroDNMDT etc.

Or to parameterize the MILP struct with the IOM configs, but I'm guessing we don't want to expose IOM to the user. E.g. like Hydro{IOM.Bin2} Hydro{IOM.DNMDT}

Anthony Costarelli and others added 5 commits April 29, 2026 09:32
Adds the bilinear (flow*head) hydro dispatch formulation and folds in
adjacent refactors merged through this branch:
  - Hydro and storage updates to IOM helpers (#97)
  - POM-to-IOM type-dispatch API migration
  - MarketBidCost / ImportExportCost static/TS split + IEC refactor
  - Shiftable-load interval indexing and validation fixes
  - HDF system serialization (#75)
  - Pin GitHub revisions; bridge IOM system-query stubs to PSY public API

Co-Authored-By: Luke Kiernan <86331877+luke-kiernan@users.noreply.github.com>
Co-Authored-By: Rodrigo Henríquez-Auba <rodrigo.henriquezauba@nrel.gov>
Co-Authored-By: Jose Daniel Lara <jdlara@berkeley.edu>
Co-Authored-By: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
# Conflicts:
#	src/PowerOperationsModels.jl
#	src/static_injector_models/hydro_generation.jl
#	src/static_injector_models/hydrogeneration_constructor.jl
Use POM.SECONDS_IN_HOUR / POM.M3_TO_KM3 in the m^3/s -> km^3
water-balance conversion instead of bare 3600 / 1e-9 literals, and
switch the volume-closure check to isapprox(...; atol = tol) with a
named tolerance and a brief comment on its sizing.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…pproximation attribute

Drop "Bin2" from the formulation type name: the bilinear-approximation
scheme is now a `DeviceModel` attribute, not part of the type identity.

Defaults reproduce the prior IOM.Bin2Config(IOM.SolverSOS2QuadConfig(4))
behavior bit-for-bit:
  bilinear_approximation     = "bin2"
  bilinear_quadratic_method  = "solver_sos2"
  bilinear_n_segments        = 4
  bilinear_add_mccormick     = nothing  # IOM struct default
  bilinear_epigraph_depth    = nothing  # IOM struct default

Sentinel `nothing` defers to the IOM constructor's default. A small
private builder pair (`_build_quadratic_config`, `_build_bilinear_config`)
maps the string attribute values to IOM config objects so callers can
swap methods without depending on InfrastructureOptimizationModels.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@acostarelli acostarelli requested review from jd-lara and rodrigomha May 13, 2026 18:00
@rodrigomha
Copy link
Copy Markdown
Contributor

A couple of things. This branch should merge to main or to the bilinear branch?

I went back and forth with Claude for some ideas on what to do here, and this is a good summary. @jd-lara will need to discuss the approach here for configuring the bilinear approx. Here is a summary:


Problem statement

The current HydroTurbineMILPBilinearDispatch API requires the user to set up to five
interdependent Dict{String,Any} keys to configure the bilinear approximation. This is verbose,
runtime-validated, and hard to discover. The goal is a simpler, type-safe surface that:

  1. Requires zero attributes for the common case (default = Bin2, SOS2, 4 segments).
  2. Requires one attribute key for any customization.
  3. Is discoverable via Julia's tab-completion and ? help system.
  4. Never exposes InfrastructureOptimizationModels types to the end user.

Evaluated options

Option 1 — Named formulation structs (Anthony's Option A, enhanced)

# One struct per top-level method
struct HydroTurbineBin2Dispatch  <: AbstractHydroDispatchFormulation end
struct HydroTurbineHybSDispatch  <: AbstractHydroDispatchFormulation end
struct HydroTurbineNMDTDispatch  <: AbstractHydroDispatchFormulation end
struct HydroTurbineDNMDTDispatch <: AbstractHydroDispatchFormulation end

get_default_attributes for each type returns method-specific keys only:

get_default_attributes(::Type{<:PSY.HydroTurbine}, ::Type{HydroTurbineBin2Dispatch}) =
    Dict{String,Any}("n_segments" => 4, "quad_method" => "solver_sos2")

Pros:

  • Zero dict keys for the most common two-parameter use case (n_segments at default)
  • Formulation choice is a type; wrong type → method error at construct_device!, not at solve

Cons:

  • Five new exported formulation types to document and maintain
  • HybS still needs "epigraph_depth" (no IOM default), so it retains an irregular surface
  • Two attributes per struct vs. one in the config-struct approach
  • Does not help users discover what can be changed without reading the docstring

Option 2 — POM config structs, single attribute key ✅ RECOMMENDED

Define a small hierarchy of config structs in src/core/ (e.g., bilinear_configs.jl):

abstract type AbstractBilinearApproxConfig end

"""Bin2 bilinear approximation (default). Linearizes f×h using a two-variable PWL scheme."""
Base.@kwdef struct Bin2Config <: AbstractBilinearApproxConfig
    n_segments::Int    = 4
    quad_method::Symbol = :solver_sos2   # :solver_sos2 | :manual_sos2 | :sawtooth | :epigraph
    add_mccormick::Bool = true
end

"""Hybrid-S bilinear approximation."""
Base.@kwdef struct HybSConfig <: AbstractBilinearApproxConfig
    n_segments::Int    = 4
    quad_method::Symbol = :solver_sos2
    epigraph_depth::Int              # required; no default in IOM
    add_mccormick::Bool = false
end

"""NMDT bilinear approximation."""
Base.@kwdef struct NMDTConfig <: AbstractBilinearApproxConfig
    depth::Int = 4
end

"""DNMDT bilinear approximation."""
Base.@kwdef struct DNMDTConfig <: AbstractBilinearApproxConfig
    depth::Int = 4
end

"""Pass the quadratic flow×head term directly (no MILP linearization)."""
struct NoBilinearApprox <: AbstractBilinearApproxConfig end

get_default_attributes becomes:

function get_default_attributes(
    ::Type{<:PSY.HydroTurbine}, ::Type{HydroTurbineMILPBilinearDispatch})
    return Dict{String,Any}("bilinear_config" => Bin2Config())
end

_build_bilinear_config is replaced by _iom_config(cfg::AbstractBilinearApproxConfig) that
dispatches on the config type — proper multiple dispatch, no string branching:

_iom_config(cfg::Bin2Config) =
    IOM.Bin2Config(
        _iom_quad_config(cfg.quad_method, cfg.n_segments),
        cfg.add_mccormick,
    )
_iom_config(cfg::NMDTConfig)  = IOM.NMDTBilinearConfig(cfg.depth)
_iom_config(cfg::DNMDTConfig) = IOM.DNMDTBilinearConfig(cfg.depth)
_iom_config(::NoBilinearApprox) = IOM.NoBilinearApproxConfig()
# ...

User API:

# Common case — no attributes needed
set_device_model!(template, HydroTurbine, HydroTurbineMILPBilinearDispatch)

# Custom segment count
set_device_model!(template, HydroTurbine, HydroTurbineMILPBilinearDispatch;
    attributes = Dict("bilinear_config" => POM.Bin2Config(n_segments = 8)))

# Switch to NMDT with depth 6
set_device_model!(template, HydroTurbine, HydroTurbineMILPBilinearDispatch;
    attributes = Dict("bilinear_config" => POM.NMDTConfig(depth = 6)))

# HybS — epigraph_depth is required
set_device_model!(template, HydroTurbine, HydroTurbineMILPBilinearDispatch;
    attributes = Dict("bilinear_config" => POM.HybSConfig(epigraph_depth = 3)))

Pros:

  • Single formulation type (HydroTurbineMILPBilinearDispatch); no type explosion
  • Single attribute key — a wrong key name now fails at get_attribute
  • Config types are tab-completable (POM.Bin2<Tab>)
  • ?POM.Bin2Config shows all keyword parameters and their defaults
  • _iom_config replaces _build_bilinear_config with clean dispatch; no string branching
  • HybSConfig makes epigraph_depth a required keyword at construction time
    (Julia will error immediately if omitted), not a runtime sentinel

Cons:

  • Adds a handful of exported config types to the POM namespace
  • Still uses Dict{String,Any} for the attribute (but exactly one key, always typed)

Option 3 — Named formulation types aliased to config-struct dispatch

Combine both: keep HydroTurbineBin2Dispatch etc. as thin types that set bilinear_config
in get_default_attributes, but dispatch through _iom_config. Gives the "best of both"
but adds all of Option 1's maintenance burden on top of Option 2's config structs.

Not recommended — the config struct alone already covers the discoverability goal.


Recommendation

Implement Option 2 (POM config structs) in a follow-up to PR 122 (or fold it into this PR
before merge).

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new hydro turbine formulation that routes turbine-flow × reservoir-head bilinear products through configurable IOM approximation schemes, plus tests around the new formulation and hydro unit conversions.

Changes:

  • Introduces and exports HydroTurbineMILPBilinearDispatch.
  • Adds default attributes and config-building logic for selectable bilinear/quadratic approximation methods.
  • Adds hydro tests for MILP bilinear dispatch plumbing and solving.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/core/formulations.jl Defines the new MILP bilinear hydro turbine formulation and adds it to shared water formulations.
src/PowerOperationsModels.jl Exports the new formulation.
src/static_injector_models/hydrogeneration_constructor.jl Includes the new formulation in turbine constructor plumbing.
src/static_injector_models/hydro_generation.jl Adds approximation attributes, config builders, and the MILP bilinear turbine-power constraint.
test/test_device_hydro_constructors.jl Adds/updates hydro tests for bounds, unit conversion, and MILP bilinear solving.
test/runtests.jl Updates commented disabled-test list entries.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1996 to +1997
min = get_variable_lower_bound(HydroTurbineFlowRateVariable, d, W),
max = get_variable_upper_bound(HydroTurbineFlowRateVariable, d, W),
Comment on lines +2001 to +2004
(
min = get_variable_lower_bound(HydroReservoirHeadVariable, res, W),
max = get_variable_upper_bound(HydroReservoirHeadVariable, res, W),
) for res in reservoirs
Comment on lines +682 to +683
# plus the MILP bilinear approximation; water-balance closure within 1e-4 km^3
# is well inside HiGHS' default feasibility tolerance for this problem size.
Comment on lines +441 to +445
"bilinear_approximation" => "bin2",
# Inner quadratic PWL method (used when bilinear_approximation ∈ {"bin2","hybs"}).
# Supported: "solver_sos2", "manual_sos2", "sawtooth", "epigraph",
# "nmdt", "dnmdt", "none".
"bilinear_quadratic_method" => "solver_sos2",
end

"""
This function define the relationship between turbined flow and power produced with a linear approximation for the bilinear product.
Comment on lines +855 to +860
hydro_head_df =
read_variables(outputs, [(HydroReservoirHeadVariable, HydroReservoir)])["HydroReservoirHeadVariable__HydroReservoir"]
hydro_spillage_df =
read_variables(outputs, [(WaterSpillageVariable, HydroReservoir)])["WaterSpillageVariable__HydroReservoir"]
hydro_inflow_df =
read_parameters(outputs, [(InflowTimeSeriesParameter, HydroReservoir)])["InflowTimeSeriesParameter__HydroReservoir"]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants