Split the units machinery out of ProductionVariableCostCurve; add LossCurve - #603
Closed
luke-kiernan wants to merge 1 commit into
Closed
Split the units machinery out of ProductionVariableCostCurve; add LossCurve#603luke-kiernan wants to merge 1 commit into
luke-kiernan wants to merge 1 commit into
Conversation
…sCurve
Loss curves on HVDC lines and converters (PowerSystems.jl) were declared as
bare `ValueCurve`s, which carry no units, so whether a curve's axes were in MW,
system base, or device base was ambiguous — and consumers disagreed in practice
(PSB builds an `InterconnectingConverter` loss curve in system base while an
open POM PR reads the same curve as device base).
Carrying units is not specific to costs, so the machinery that does it is now
split out one level:
ValueCurveWithUnits{T, U} # value_curve + units type parameter
├── ProductionVariableCostCurve # adds vom_cost
│ ├── CostCurve
│ └── FuelCurve
└── LossCurve
`ValueCurveWithUnits` owns everything the two families share — the units type
parameter and `get_power_units`, the value-curve accessors, convexity,
equality/hashing, `serialize`/`deserialize` including the `power_units` key, and
the expanded `show`. `ProductionVariableCostCurve` keeps only what makes a curve
a *cost*: `get_vom_cost` and the cost-specific per-field deserializers.
That division is load-bearing, not cosmetic. Curve-valued cost fields
(`ThermalGenerationCost.variable`, `HydroGenerationCost.variable`) are typed on
`ProductionVariableCostCurve`, so they still reject a `LossCurve`; and losses
have no VOM cost, so inheriting `get_vom_cost` would have promised a field that
isn't there. Sharing at the `ValueCurveWithUnits` level gets the common
implementation without either problem.
No behavior change for `CostCurve`/`FuelCurve`: the methods moved to the
supertype, none were removed or altered.
Part of the fix for PowerSystems.jl#1728.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the unit-system type-parameter machinery out of ProductionVariableCostCurve into a new shared supertype (ValueCurveWithUnits) and introduces LossCurve so loss-related curves can carry an explicit (dispatchable) x-axis unit system, addressing ambiguous axes for HVDC loss curves.
Changes:
- Added
ValueCurveWithUnits{T,U}to own shared accessors, equality/hash, serialization, andshowbehavior for unit-parameterized curves. - Updated
ProductionVariableCostCurve{T,U}to subtypeValueCurveWithUnits{T,U}and kept only cost-specific functionality (e.g.,vom_costfield deserialization). - Added
LossCurve{T,U}plus new tests covering construction, display, serialization round-trips, andzerobehavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/test_cost_functions.jl | Adds LossCurve test coverage (construction, units typing, show output, serialization, zero). |
| src/value_curve_with_units.jl | Introduces new shared abstract supertype and common methods (accessors, TS helpers, equality/hash, serialization, show). |
| src/production_variable_cost_curve.jl | Re-bases PVCC on ValueCurveWithUnits and moves shared logic out; retains PVCC-specific deserializers and compact show. |
| src/loss_curve.jl | Adds the new LossCurve type, constructors, AnyLossCurve, zero, and compact show. |
| src/InfrastructureSystems.jl | Wires new source files into module load order via include. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Base.isequal(a::T, b::T) where {T <: ValueCurveWithUnits} = isequal_from_fields(a, b) | ||
|
|
||
| Base.hash(a::ValueCurveWithUnits, h::UInt) = hash_from_fields(a, h) |
luke-kiernan
marked this pull request as draft
August 6, 2026 16:19
Contributor
Author
|
Reverted to draft--revisit after the schema stuff lands, so that the folks working on integrating that stuff don't have to deal with a moving target. |
Member
|
@luke-kiernan you can restart this now |
Contributor
Author
|
Superseded by #621, which reimplements this on top of current |
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.
Supports PowerSystems.jl#1728, where loss curves on HVDC lines and converters are bare
ValueCurves carrying no units, leaving their axes ambiguous.Carrying units isn't specific to costs, so that machinery is split out one level:
The new supertype owns what both families share (units parameter, value-curve accessors, equality/hashing, serialization, display).
ProductionVariableCostCurvekeeps only what makes a curve a cost:get_vom_costand the cost-specific field deserializers.LossCurveis deliberately not underProductionVariableCostCurve:ThermalGenerationCost.variable/HydroGenerationCost.variableare typed on it, andget_vom_costis defined on it — a loss curve has no VOM cost.No behavior or serialization change for
CostCurve/FuelCurve; methods moved, none removed or altered.Tests: 1462 pass. The
Test generated structsfailure is pre-existing onIS4(verified against a clean baseline, unrelated).Pairs with a PowerSystems.jl PR against
psy6.🤖 Generated with Claude Code