Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/InfrastructureOptimizationModels.jl
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ export add_to_expression!
export add_constant_to_jump_expression!
export add_proportional_to_jump_expression!
export add_linear_to_jump_expression!
export add_device_terms_to_expression!
# Cost term helpers (generic objective function building blocks)
export add_cost_term_to_expression!
export add_cost_term_invariant!
Expand Down
60 changes: 60 additions & 0 deletions src/common_models/add_jump_expressions.jl
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,63 @@ function add_linear_to_jump_expression!(
add_proportional_to_jump_expression!(expression, var, multiplier)
return
end

"""
Generic driver for device-injection `add_to_expression!` methods.

For each device in `devices` and each time step, adds a proportional term to one or
more expression entries. Two closures separate the axes that distinguish these
methods, so the network-model and the term-source can vary independently while the
loop itself is written once:

- `targets_fn(d)` returns a 1- or 2-element tuple of `(expression_matrix, row_index)`
targets identifying which expression entries device `d` contributes to: one entry
for single-bus/area/system network models, two for PTDF/AreaPTDF (a nodal entry
plus a system/area entry). This is where the network-model dependence lives.
- `term_fn(d)` returns a per-device closure `t -> (value, multiplier)` giving the
term added at each time step, where `value` is a JuMP variable/parameter
reference or a `Float64` constant and `multiplier::Float64`. This is where the
variable/parameter/constant source lives. Per-device setup (name lookups, bounds,
warnings) belongs in `term_fn` so it runs once per device rather than per step.

The same `value * multiplier` term is added to every target via
[`add_proportional_to_jump_expression!`](@ref).
"""
function add_device_terms_to_expression!(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not convinced passing a function here is a clean option. This function can't never compile and cause invalidations. This implementation is very pythonic

@luke-kiernan luke-kiernan Jun 22, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, there's definitely some code smell. I agree that this solution feels clunky. I didn't check the compilation consequences of having a function stub in a hotloop.

I can remove the function closures and eliminate add_device_terms_to_expression! here in IOM. However, I'd like to keep the target resolution code in the POM PR. Concretely: in the variable case, we'd have

  function add_to_expression!(container, ::Type{T}, ::Type{U}, devices, ::DeviceModel{V,W}, network_model) where {...}
      variable = get_variable(container, U, V)
      mult = get_variable_multiplier(U, V, W)
      time_steps = get_time_steps(container)
      for d in devices
          targets = _balance_expression_targets(container, T, network_model, d)
          name = PSY.get_name(d)
          for t in time_steps
              _apply_term_to_targets!(targets, variable[name, t], mult, t)
          end
      end
      return
  end

whereas right now on main, the network model specifics inside _balance_expression_targets are copy-pasted (ref-bus lookup for CopperPlate, area lookup for AreaBalance, etc.) and _apply_term_to_targets! is 1 (non-PTDF) or 2 (PTDF) calls to add_proportional_to_jump_expression!

@luke-kiernan luke-kiernan Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I didn't check the compilation consequences of having a function stub in a hotloop.

Wait. I saw _apply_term_to_targets!(::Tuple{}, ::_BalanceTermValue, ::Float64, ::Int) = nothing and thought I had a stub function, but no, this is tail recursion. I'm convinced there is no actual compilation or type stability issue here.

Full reasoning For a single call site (one concrete `T` , `network_model`, term source):
  1. targets_fn::F, term_fn::G — the where {F<:Function, G<:Function} forces Julia to specialize the driver per closure type. F/G are concrete inside any given specialization, not abstract Function. No dynamic dispatch to call them.
  2. targets = targets_fn(d) — dispatches to exactly one _balance_expression_targets method (T and network_model are concrete), so the tuple structure (1- vs 2-element, and each element's index type) is fixed and concrete.
  3. _apply_term_to_targets! recursion — operates on a concrete 1-or-2 tuple; Base.tail shortens it by one each step, terminating at the ::Tuple{} method. This fully unrolls at compile time and handles the heterogeneous 2-tuple (system-keyed + bus-keyed) correctly.
  4. scale::S as a positional type parameter — isnothing(scale) folds to a compile-time constant, so the dead branch is pruned. No instability, no scale(d) dynamic call.
  5. Closure captures — I had Claude check each closure: name, multiplier, variable, refs are each assigned exactly once before the inner t -> … is built. No reassignment of a captured variable → no Core.Box, no boxing instability.

Two closures branch and return different inner closures, t -> ... lambdas with distinct types. That's a little messy: term = term_fn(d) infers as Union{Closure1, Closure2}. Thanks to union-splitting, though, the performance cost should be next to zero.

I can go run some profiling to confirm. If the code smell is too much, I'm on board with letting this go and sticking with what I suggested above...but I'm pretty confident that compilation/invalidations here aren't an issue.

container::OptimizationContainer,
targets_fn::F,
term_fn::G,
devices::Union{Vector{D}, IS.FlattenIteratorWrapper{D}},
) where {F <: Function, G <: Function, D}
time_steps = get_time_steps(container)
for d in devices
targets = targets_fn(d)
term = term_fn(d)
for t in time_steps
value, multiplier = term(t)
_apply_term_to_targets!(targets, value, multiplier, t)
end
end
return
end

# Apply a term to each `(expression_matrix, row_index)` target. A device contributes to
# either one target (single-bus/area/system network models) or two (PTDF/AreaPTDF: a
# nodal entry plus a system/area entry). Tail recursion ensures type stability for
# the hetereogeneous length 2 tuples.
Comment on lines +100 to +101
const _BalanceTermValue = Union{Float64, JuMP.AbstractJuMPScalar}

_apply_term_to_targets!(::Tuple{}, ::_BalanceTermValue, ::Float64, ::Int) = nothing

function _apply_term_to_targets!(
targets::Tuple,
value::_BalanceTermValue,
multiplier::Float64,
t::Int,
Comment on lines +102 to +110
)
expression, row = targets[1]
add_proportional_to_jump_expression!(expression[row, t], value, multiplier)
# perf note: only called with length 1 or 2 tuples, but writing separate methods
# has no advantage, because compiler unrolls the tail recursion.
return _apply_term_to_targets!(Base.tail(targets), value, multiplier, t)
end
Loading