Add tolerance-driven bilinear approximation config API

[acostarelli]

Introduces a type-safe, tolerance-driven configuration API for selecting the bilinear approximation method used by HydroTurbineMILPBilinearDispatch, replacing the previous multi-key Dict{String,Any} attribute surface with a single "bilinear_config" attribute holding a typed config struct.

Changes Made

• New src/core/bilinear_configs.jl: Defines a generic AbstractBilinearApproxConfig hierarchy (not hydro-specific) with config structs for each supported scheme:
  • Bin2Config — default; Bin2 bilinear approximation with selectable inner quadratic method
  • HybSConfig — Hybrid-S approximation
  • NMDTConfig — NMDT approximation
  • DNMDTConfig — DNMDT approximation
  • NoBilinearApprox — passes the product directly to the solver (no MILP linearization)
  • Inner quad marker types: SolverSOS2, ManualSOS2, Sawtooth, Epigraph, NMDTQuad, DNMDTQuad, with Bin2Quad/HybSQuad union type constraints matching IOM's supported combinations
• Tolerance-based discretization: All config structs accept a tolerance::Float64 (default 1e-2); the discretization depth is derived automatically from the tolerance and the two variables' domain widths via IOM.tolerance_depth at constraint-build time — no manual depth or segment-count knob exposed to the user
• _validate_tolerance: Construction-time guard rejecting non-finite or non-positive tolerances with a clear ArgumentError
• _iom_config bridge: Dispatches on AbstractBilinearApproxConfig subtypes to produce the corresponding IOM config, replacing prior string-branching logic
• HydroTurbineMILPBilinearDispatch: Default attribute is Dict("bilinear_config" => Bin2Config()); finite flow and head bounds are validated before calling IOM, with clear error messages
• Project.toml: InfrastructureOptimizationModels pinned to main

User API

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

# Tighter tolerance with default Bin2/SolverSOS2
set_device_model!(template, HydroTurbine, HydroTurbineMILPBilinearDispatch;
    attributes = Dict("bilinear_config" => POM.Bin2Config(tolerance = 1e-3)))

# Switch to NMDT
set_device_model!(template, HydroTurbine, HydroTurbineMILPBilinearDispatch;
    attributes = Dict("bilinear_config" => POM.NMDTConfig(tolerance = 1e-2)))

Testing

• Added constructor tests for HydroTurbineMILPBilinearDispatch plumbing (variable/constraint presence)
• Added solve test with HiGHS verifying the formulation produces a feasible solution
• Added hydro unit conversion tests

[rodrigomha]

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).

[Copilot]

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.

[rodrigomha]

@acostarelli Can you fix the conflicts here? Also, can we have a docstring or some documentation on a rough idea on how tolerance translates to depth or segments

[acostarelli]

@rodrigomha Yes, I'm still adjusting the IOM side.

[acostarelli]

It is not just for hydro stuff. Make sure none of this is hydro-specific. Yes that's the name of the branch, but it's not the only use-case for this stuff.

[acostarelli]

Also hydro specific

[acostarelli]

Hydro specific

[acostarelli]

f and h are short for flow and head which, again, are hydro specific

[acostarelli]

Please make sure these unions and their doc-strings are up-to-date with what IOM says.

[Copilot]

Pull request overview

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

[rodrigomha]

It seems that we are going to go with the attributes but using tolerance rather than segments and depth.

@copilot Review this again

[Copilot]

It seems that we are going to go with the attributes but using tolerance rather than segments and depth.

@copilot Review this again

Reviewed the current state of the PR. Here's a summary:

What's in good shape:

• bilinear_configs.jl uses generic x × y language throughout — no hydro-specific references remain in the config types or docstrings.
• Bin2Quad and HybSQuad union constraints match IOM exactly: Bin2Config excludes EpigraphQuadConfig (one-sided-under) while accepting both one-sided-over and two-sided inner quads; HybSConfig restricts to SolverSOS2, ManualSOS2, and Sawtooth only, consistent with IOM's tolerance_depth(::Type{HybSConfig{Q}}) dispatch.
• _validate_tolerance guards all four config structs at construction time.
• Project.toml now pins IOM to main.
• Finite flow/head bounds are validated with clear error messages before calling _iom_config.

One remaining gap to be aware of:
get_attribute(model, "bilinear_config") on line 1869 has no upfront type check — a wrong attribute type will surface as a MethodError in _iom_config. Adding an isa AbstractBilinearApproxConfig guard with a clear message would improve the user-facing error. That said, the previous review thread on this was marked resolved, so flagging for awareness rather than a blocker.

[rodrigomha]

@acostarelli I think @jd-lara prefers that we don't define new structs and just go back to using attributes as before:

return Dict{String, Any}(
        # Top-level bilinear approximation scheme.
        # Supported: "bin2", "hybs", "nmdt", "dnmdt", "none".
        "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",
        # Maximum approximation gap. Combined with the per-device max_delta
        # to size the discretization automatically.
        "bilinear_tolerance" => 1e-2,
        # `nothing` ⇒ defer to IOM struct default
        # (Bin2Config: true; HybSConfig: false; ignored otherwise).
        "bilinear_add_mccormick" => nothing,
        # `nothing` ⇒ defer to IOM struct default
        # (HybSConfig has no default and must be overridden; NMDT/DNMDT
        #  quadratic: 3*depth; ignored otherwise).
        "bilinear_epigraph_depth" => nothing,
    )
end

@jd-lara Can you confirm if you prefer going back to this, or do you want to use the proposed approach here of adding structs.

[jd-lara]

@acostarelli I think @jd-lara prefers that we don't define new structs and just go back to using attributes as before:

return Dict{String, Any}(
        # Top-level bilinear approximation scheme.
        # Supported: "bin2", "hybs", "nmdt", "dnmdt", "none".
        "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",
        # Maximum approximation gap. Combined with the per-device max_delta
        # to size the discretization automatically.
        "bilinear_tolerance" => 1e-2,
        # `nothing` ⇒ defer to IOM struct default
        # (Bin2Config: true; HybSConfig: false; ignored otherwise).
        "bilinear_add_mccormick" => nothing,
        # `nothing` ⇒ defer to IOM struct default
        # (HybSConfig has no default and must be overridden; NMDT/DNMDT
        #  quadratic: 3*depth; ignored otherwise).
        "bilinear_epigraph_depth" => nothing,
    )
end

@jd-lara Can you confirm if you prefer going back to this, or do you want to use the proposed approach here of adding structs.

correct, lets keep it as attributes for the public API

[jd-lara]

These two aren't needed

"bilinear_add_mccormick" => nothing,
        # `nothing` ⇒ defer to IOM struct default
        # (HybSConfig has no default and must be overridden; NMDT/DNMDT
        #  quadratic: 3*depth; ignored otherwise).
        "bilinear_epigraph_depth" => nothing, 

The epigraph depth should be based on the tolerance too

[github-actions]

Performance Results

Version	Precompile Time
Main	2.938933564
This Branch	3.103351065

Version	Build Time
Main-Build Time Precompile	85.898741713
Main-Build Time Postcompile	2.044385915
This Branch-Build Time Precompile	84.207998791
This Branch-Build Time Postcompile	1.87959586

Version	Solve Time
Main-Solve Time Precompile	FAILED TO TEST
Main-Solve Time Postcompile	FAILED TO TEST
This Branch-Solve Time Precompile	FAILED TO TEST
This Branch-Solve Time Postcompile	FAILED TO TEST

[acostarelli]

These "attribute config bridge" tests are pointless remove them all.

[acostarelli]

Not sure what _capability means here. Also I'm not a fan of "pq" being used as the main name here. Maybe I'm not familiar with power systems enough, but does "PQ" communicate the constraint that "P^2+Q^2<=X" ?

[Copilot]

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

[acostarelli]

Thoughts?

[jd-lara]

Code review

Found 1 issue:

1. Inverted bound-validation guard in the hydro bilinear constraint. The guard fires only when bilinear_method == "none" (the exact NLP path, which never sizes a discretization), but the error message says the bounds are needed "to size the bilinear approximation." For the approximation paths ("bin2", "nmdt", etc.) — the ones that actually feed flow_delta/head_delta into _build_bilinear_config — the guard does not fire, so a turbine without outflow_limits (flow_ub === nothing) skips the helpful error and crashes at flow_delta = flow_ub - flow_lb with a nothing - Float64 MethodError. The condition should be bilinear_method != "none". The same inversion applies to the head-bound guard at line 1842 (isnothing(b.max) → crash at head_delta = maximum(b.max - b.min ...)).

PowerOperationsModels.jl/src/static_injector_models/hydro_generation.jl

Lines 1824 to 1833 in 43465c2

	flow_lb = get_variable_lower_bound(HydroTurbineFlowRateVariable, d, W)
	flow_ub = get_variable_upper_bound(HydroTurbineFlowRateVariable, d, W)
	bilinear_method == "none" && isnothing(flow_ub) &&
	error(
	"HydroTurbineBilinearDispatch requires finite turbine outflow " *
	"limits to size the bilinear approximation, but turbine \"$(name)\" " *
	"has no `outflow_limits`. Set finite outflow limits or use a " *
	"different hydro turbine formulation.",
	)
	flow_delta = flow_ub - flow_lb

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}