Cost expression refactor, parameter broadcasting, and PF headroom slack porting#97
Open
acostarelli wants to merge 7 commits into
Open
Cost expression refactor, parameter broadcasting, and PF headroom slack porting#97acostarelli wants to merge 7 commits into
acostarelli wants to merge 7 commits into
Conversation
Contributor
|
Performance Results This branch |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors objective-function cost bookkeeping by introducing constituent cost expression types (fuel/VOM/fixed/startup/shutdown) with automatic propagation into an aggregate ProductionCostExpression, while also adding parameter-setting/indexing fast paths and extending the power-flow-in-the-loop API surface.
Changes:
- Added
ConstituentCostExpressionsubtypes and propagation logic so constituent costs can be tracked separately while still contributing toProductionCostExpression. - Updated multiple objective-function builders to write into the appropriate constituent expression types (e.g., fuel vs fixed vs startup/shutdown).
- Introduced indexing/broadcasting utilities and parameter-container “raw data” accessors intended to speed up parameter writes; updated power-flow evaluation hook to pass
system.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/indexing.jl | Refactors index expansion and adds fast paths for assignment/fixing into DenseAxisArrays. |
| src/objective_function/start_up_shut_down.jl | Routes startup/shutdown cost terms to new constituent expression types. |
| src/objective_function/quadratic_curve.jl | Routes quadratic/linear variable cost bookkeeping to FuelCostExpression with propagation. |
| src/objective_function/proportional.jl | Routes proportional cost bookkeeping to FixedCostExpression. |
| src/objective_function/piecewise_linear.jl | Routes piecewise variable cost bookkeeping to FuelCostExpression. |
| src/objective_function/linear_curve.jl | Updates proportional-cost helper usage to specify target constituent expression type. |
| src/objective_function/cost_term_helpers.jl | Adds propagation hook into ProductionCostExpression for constituent expressions; extends proportional helper API. |
| src/objective_function/common.jl | Ensures add_cost_to_expression! also propagates; routes time-varying fuel costs to FuelCostExpression. |
| src/InfrastructureOptimizationModels.jl | Exports new cost expression types and power-flow extension points/helpers. |
| src/core/standard_variables_expressions.jl | Defines new cost expression types and generalizes cost-expression container creation. |
| src/core/parameter_container.jl | Adds raw .data accessors and integer-indexed fast-path setters for parameters/multipliers. |
| src/core/optimization_container.jl | Calls solve_powerflow! with system to support PF input updates. |
| src/core/network_model.jl | Relaxes power_flow_evaluation storage to accept external evaluator types. |
| src/common_models/interfaces.jl | Documents updated PF solve extension signature and adds additional PF-related extension points. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+113
to
+116
| get_parameter_array_data(c::ParameterContainer) = get_parameter_array(c).data | ||
| get_multiplier_array(c::ParameterContainer) = c.multiplier_array | ||
| # Same shortcut for the multiplier array — used by the integer-indexed fast path. | ||
| get_multiplier_array_data(c::ParameterContainer) = get_multiplier_array(c).data |
Collaborator
There was a problem hiding this comment.
Agreed. This seems like an actual problem.
Comment on lines
1254
to
1257
| for (i, pf_e_data) in enumerate(get_power_flow_evaluation_data(container)) | ||
| @debug "Processing power flow $i" | ||
| solve_powerflow!(pf_e_data, container) | ||
| solve_powerflow!(pf_e_data, container, system) | ||
| for key in pf_aux_var_keys |
Collaborator
There was a problem hiding this comment.
Breaking compatibility is fine. One nitpick: in PF, we define solve_power_flow!(pf_e_data), 1 argument and 3 words. I'd probably make the names uniform but leave the arguments.
Comment on lines
+69
to
74
| # Stored as Vector{<:Any} so external power flow types (e.g., | ||
| # `PowerFlows.PowerFlowEvaluationModel`) that don't subtype IS's | ||
| # `AbstractPowerFlowEvaluationModel` can be supplied here. The actual | ||
| # validation/dispatch happens in `add_power_flow_data!` in POM/PowerFlowsExt. | ||
| power_flow_evaluation::Vector | ||
| subsystem::Union{Nothing, String} |
Collaborator
There was a problem hiding this comment.
We should just make PF.PowerFlowEvaluationModel a subtype of IS.AbstractPowerFlowEvaluationModel, so that the vector can be typed.
luke-kiernan
requested changes
May 7, 2026
Comment on lines
+113
to
+116
| get_parameter_array_data(c::ParameterContainer) = get_parameter_array(c).data | ||
| get_multiplier_array(c::ParameterContainer) = c.multiplier_array | ||
| # Same shortcut for the multiplier array — used by the integer-indexed fast path. | ||
| get_multiplier_array_data(c::ParameterContainer) = get_multiplier_array(c).data |
Collaborator
There was a problem hiding this comment.
Agreed. This seems like an actual problem.
| var = get_variable(container, T, V)[name, time_period] | ||
|
|
||
| cost = if quadratic_term_per_unit >= eps() | ||
| if quadratic_term_per_unit >= eps() |
Collaborator
There was a problem hiding this comment.
Oh pre-existing issue: abs(quadratic_term_per_unit) >= eps() would be safer.
Comment on lines
1254
to
1257
| for (i, pf_e_data) in enumerate(get_power_flow_evaluation_data(container)) | ||
| @debug "Processing power flow $i" | ||
| solve_powerflow!(pf_e_data, container) | ||
| solve_powerflow!(pf_e_data, container, system) | ||
| for key in pf_aux_var_keys |
Collaborator
There was a problem hiding this comment.
Breaking compatibility is fine. One nitpick: in PF, we define solve_power_flow!(pf_e_data), 1 argument and 3 words. I'd probably make the names uniform but leave the arguments.
Comment on lines
+69
to
74
| # Stored as Vector{<:Any} so external power flow types (e.g., | ||
| # `PowerFlows.PowerFlowEvaluationModel`) that don't subtype IS's | ||
| # `AbstractPowerFlowEvaluationModel` can be supplied here. The actual | ||
| # validation/dispatch happens in `add_power_flow_data!` in POM/PowerFlowsExt. | ||
| power_flow_evaluation::Vector | ||
| subsystem::Union{Nothing, String} |
Collaborator
There was a problem hiding this comment.
We should just make PF.PowerFlowEvaluationModel a subtype of IS.AbstractPowerFlowEvaluationModel, so that the vector can be typed.
- restrict get_parameter_array_data/get_multiplier_array_data to
ParameterContainer{<:DenseAxisArray} so SparseAxisArray-backed
containers fail with MethodError instead of `no field data`
- remove backward-compat 5-arg add_proportional_cost_invariant! shim
(all callers already pass the constituent expression type)
- guard quadratic-term test with abs() so negative quadratic terms
still take the quadratic branch
- add ProductionCostExpression propagation testset covering every
entry point, the multi-constituent sum invariant, non-constituent
paths to the JuMP objective, and the unregistered-container no-op
- update test_linear_curve.jl callers to the new 7-arg signature
Co-Authored-By: Claude Opus 4.7 <[email protected]>
luke-kiernan
approved these changes
May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sienna-Platform/PowerSimulations.jl#1564
Sienna-Platform/PowerSimulations.jl#1609
Sienna-Platform/PowerSimulations.jl#1566