Lk/flow sign trait#117
Closed
luke-kiernan wants to merge 13 commits into
Closed
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Rename solve_powerflow! → solve_power_flow! to match PowerFlows naming
(kiernan).
- Fix aux-var readback guard to compare AuxVarKey entry-type, not the
whole key, against branch_aux_vars / bus_aux_vars; add a regression
test covering an unsupported PowerFlowAuxVariable key.
- Rename _get_variable_if_exists → _get_cost_if_exists so it no longer
reads as an optimization-variable accessor.
- Route LinearCurve fuel cost and renewable curtailment cost through
the new IOM helpers add_cost_term_to_expression! and
add_cost_term_invariant!; route compact-fuel SOS dispatch through
IOM._determine_bin_lhs to drop the duplicated three-way branch.
- Replace Ref(x) with singleton tuples (x,) in broadcast calls for
stack allocation; drop unnecessary Float64(multiplier) casts.
- Tighten devices argument on _add_time_series_parameters! to
Union{Vector{D}, IS.FlattenIteratorWrapper{D}}.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`findlast` returns `nothing` when no `PowerFlowEvaluationData` entry has solved, which previously bubbled up as an opaque `MethodError` from `datas[nothing]`. Raise an explicit error and mark the broader handling as a FIXME pending the PF-failure design discussion. Addresses kiernan's review comment on PR #112. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Wire `get_variable_multiplier` default through IOM's flow_sign trait; delete ~30 redundant ±1.0 overrides. NaN catch-alls become loud errors. Rename 5-arg storage AS overload to `get_reserve_balance_factor` (0/1 incidence, distinct from sign). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
luke-kiernan
changed the base branch from
main
to
ac/psi-costexp-parambroad-pfslack
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors how variable multipliers are determined when building expressions by switching the default get_variable_multiplier behavior to consult IOM.flow_sign (Injection/Withdrawal/Unsigned), and replaces several prior NaN “stubs” with an explicit erroring fallback to avoid silently poisoning JuMP expressions.
Changes:
- Update the default
get_variable_multiplierto derive signs fromIOM.flow_sign, and add an_unsupported_multiplierhard-error helper for intentionally-undefined triples. - Remove a number of device/formulation-specific
get_variable_multipliermethods that are now redundant under the flow-sign trait, and defineIOM.flow_signfor key POM-owned slack/pump variable types. - Rename Storage ancillary-service “multiplier” helper to
get_reserve_balance_factorand adjust its call sites; update IOM dependency rev to the required branch.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Project.toml | Points IOM source at lk/flow-sign-trait branch required for the new trait-based multiplier behavior. |
| test/Project.toml | Mirrors IOM source override for the test environment. |
| src/core/interfaces.jl | Implements trait-driven default get_variable_multiplier and introduces _unsupported_multiplier hard-error fallback. |
| src/core/variables.jl | Adds IOM.flow_sign definitions for POM-owned slack variables and ActivePowerPumpVariable. |
| src/network_models/network_slack_variables.jl | Removes explicit slack multiplier methods (now handled via flow_sign). |
| src/services_models/transmission_interface.jl | Removes explicit interface slack multipliers (now handled via flow_sign). |
| src/services_models/reserves.jl | Replaces NaN multiplier fallback with _unsupported_multiplier error for Reserve devices (with specific overrides retained). |
| src/services_models/agc.jl | Replaces NaN multiplier fallback with _unsupported_multiplier error for AGC (file currently not included, but updated consistently). |
| src/static_injector_models/thermal_generation.jl | Removes redundant ThermalGen multiplier defaults now covered by the trait-driven default. |
| src/static_injector_models/source.jl | Removes Source multipliers now expected to be handled via variable flow-sign. |
| src/static_injector_models/renewable_generation.jl | Removes redundant RenewableGen multiplier default. |
| src/static_injector_models/reactivepower_device.jl | Removes redundant SynchronousCondenser multiplier default. |
| src/static_injector_models/hydro_generation.jl | Removes redundant HydroGen multipliers and relies on flow-sign (with pump withdrawal handled via flow_sign). |
| src/twoterminal_hvdc_models/TwoTerminalDC_branches.jl | Replaces NaN multiplier stub with _unsupported_multiplier error for unsupported HVDC flow variable case. |
| src/mt_hvdc_models/HVDCsystems.jl | Removes redundant MT-HVDC multiplier defaults now covered by the trait-driven default. |
| src/ac_transmission_models/AC_branches.jl | Removes redundant PhaseShifterAngle multiplier default now covered by the trait-driven default. |
| src/energy_storage_models/storage_models.jl | Makes Storage default multiplier error loudly for unsupported variables; renames ancillary-service balance helper to get_reserve_balance_factor and updates call sites. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
84
to
+108
| """ | ||
| Get the multiplier for a variable type when adding to an expression. | ||
| Default implementation returns 1.0. | ||
| """ | ||
| get_variable_multiplier( | ||
| ::Type{<:IS.Optimization.VariableType}, | ||
| ::Type{<:IS.InfrastructureSystemsComponent}, | ||
| ::Type{<:IOM.AbstractDeviceFormulation}, | ||
| ) = 1.0 | ||
|
|
||
| Default consults [`IOM.flow_sign`](@ref) on the variable type: variables marked | ||
| `Injection` contribute `+1.0`, `Withdrawal` contributes `-1.0`, and `Unsigned` | ||
| falls back to `1.0`. Device-driven sign rules (e.g. anything on `PSY.ElectricLoad`) | ||
| should be expressed as more-specific dispatches that override this default. | ||
| """ | ||
| function get_variable_multiplier( | ||
| ::Type{V}, | ||
| # second arg is any system component, or `PSY.System` which is a ComponentContainer | ||
| ::Type{<:Union{IS.InfrastructureSystemsComponent, IS.ComponentContainer}}, | ||
| ::Type, # could be PowerModel (network) or DeviceFormulation (device) | ||
| ) where {V <: IS.Optimization.VariableType} | ||
| return IOM.multiplier_from_sign(IOM.flow_sign(V)) | ||
| end | ||
|
|
||
| # Hard-error fallback for variable/device/formulation triples that intentionally | ||
| # have no defined multiplier (must be specialized at the call site). Used by | ||
| # devices like Storage/AGC/Reserve/TwoTerminalHVDC to fail loudly instead of | ||
| # returning NaN, which silently poisons JuMP expressions. | ||
| @noinline _unsupported_multiplier(V, D, F) = error( | ||
| "get_variable_multiplier not implemented for variable $V on $D under $F. " * | ||
| "This combination must be specialized.", | ||
| ) |
|
Performance Results
|
Mirror the IOM rename in POM's flow_sign dispatches and docstring. Add test/test_flow_sign_multipliers.jl pinning power-balance multiplier conventions from first principles. Address Copilot review on #117. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
luke-kiernan
force-pushed
the
lk/flow-sign-trait
branch
from
f890604 to
0fc5028
Compare
Collaborator
Author
|
I think I'll just close this: long term it'd be nice to have something self-documenting, but there's more pressing design changes in the pipeline |
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.
Clean up
get_variable_multiplier. Stacked on top of #112 for convenience, but merge after. Depends on #98 in IOM