Continue the PSI port backlog: loss curves, EnergyLimitFeedforward, and module-independent container keys - #289
Merged
Merged
Conversation
IOM #164 (share_template_references!) is merged, so the [sources] pins go back to rev = "main" in the root and test projects. The docs project follows the upstream PowerTimeSeriesOpenAPIModels -> InfrastructureTimeSeriesOpenAPIModels rename, which is what broke the Documentation CI job. feedforward_interface.jl's header still claimed the event infrastructure lived in PowerSimulations and had not been ported; it describes the real fallbacks now. get_empty_timeseries_mapping gains a typed error for a non-event PSY.Contingency instead of a bare MethodError, and outage_power_offset's whole-duration semantics are documented as the live behavior now that PSI #1664 delegates both offset writes here.
IS4 absorbed lk/loss-curve-units-v2, so get_loss, get_loss_function and get_converter_loss_from/_to now return a LossCurve wrapper that declares its own unit system. get_proportional_term and get_constant_term are defined on the inner curve types only, so every reader had to unwrap; twelve sites needed it, including the two HVDCVSCConverterPowerConstraint methods. Curve-shape branching moves from isa checks to dispatch on the unwrapped ValueCurve: _hvdc_linear_loss_terms and _hvdc_pwl_len_segments replace the "only accepts LinearCurve" guards and the "Should not be here" fallback, each with a method that errors and names the offending type. _get_quadratic_term's untyped `= 0.0` fallback was a silent-failure hazard: a LossCurve-wrapped QuadraticCurve hit it and returned zero, dropping the a*I^2 term and disarming the guard that exists to refuse quadratic curves under LinearLossConverter. It now dispatches per curve type and errors on anything else. _loss_curve_value converts a curve to system base from whatever base it declares, replacing the "assume the curve is authored in device base" base_factor. This reproduces the old arithmetic exactly for a DeviceBaseUnit curve and corrects it for the NaturalUnit curves the data actually carries, which the previous code scaled as if they were per-unit. The unit-system dispatch is resolved once per device, above the time loop, so the abstract field type PSY declares keeps its cost at build time and the per-step arithmetic stays plain Float64.
…e-independent EnergyLimitFeedforward is the storage twin of ReservoirLimitFeedforward: same FeedforwardIntegralLimitConstraint, same per-block trench math, differing only in the parameter it reads. EnergyLimitParameter already existed, so the port is the struct plus a shared _add_integral_limit_constraints! that both types dispatch into, mirroring the _add_energy_target_constraints! split. The two testsets fenced in test_storage_device_models are live again; they had been written against BookKeeping and BatteryAncillaryServices, which POM does not have, so they run against StorageDispatchWithReserves over both battery fixtures. The source-conflict guard dispatched on a four-member Union of concrete feedforward types, which EnergyLimitFeedforward would have grown to five. It is a trait now, so each participating type opts in with one method. The property cuts across the hierarchy rather than along it -- WaterLevelBudgetFeedforward does not opt in -- so a shared supertype would have been the wrong tool. The mixed-type fallback is kept: two feedforwards of different types read different containers and cannot collide. meta strings interpolated a DataType directly, which renders module-qualified whenever the name is not visible in the active module. The test runner executes each file in a fresh module, so these keys were latently unstable; nameof makes them independent of the caller's module. The slack lookup in the bound- feedforward constraint had to move in lockstep with its registration, and the test expectations that build the same keys had to follow, or the read side asks for a qualified key the write side no longer stores. Also from the port reviews: the one-step storage shortage slack accepts a Vector of devices rather than only a FlattenIteratorWrapper, so a Vector caller cannot fall through to the unbounded full-horizon generic; and the hydro coefficient testset asserts its constraint set through a dispatched predicate instead of isa.
The formulation library tabulated five of POM's feedforward types and the explanation page gave guidance for four. The reservoir target and limit, water level budget, hydro usage limit and new energy limit feedforwards now have rows carrying their parameter, constraint container and exact constraint expression, and a guidance bullet each. Two behaviours are worth stating because neither is visible from the table: ReservoirLimitFeedforward dispatches on any PSY.Component despite its name, and HydroUsageLimitFeedforward reads ActivePowerVariable directly and nets served regulation reserves when the device model carries a service model.
Member
Author
|
@luke-kiernan please focus the review on the use of the loss functions please. |
luke-kiernan
left a comment
luke-kiernan
left a comment
Collaborator
There was a problem hiding this comment.
Started reviewing at-large then remembered that you wanted me to focus on loss curves. I see it's been merged, but here's my comments
| step — use it when a longer-horizon model has fixed a reservoir level partway through this | ||
| model's horizon, not only at the end. | ||
| - **`ReservoirLimitFeedforward`** — bound the sum of a variable over consecutive blocks of | ||
| `number_of_periods` steps to a per-block limit from the state. Reach for it when a |
Collaborator
There was a problem hiding this comment.
bound the sum of a variable over consecutive blocks of
number_of_periodssteps to a per-block limit from the state.
Confusing. If you're summing over blocks, then how are you enforcing a per block limit on the resulting total?
| - **`HydroUsageLimitFeedforward`** — bound a hydro unit's cumulative active power, summed | ||
| over the full horizon, to an energy usage limit from the state. Reach for it to cap total | ||
| hydro generation against a state-recorded allocation; the recommended source is the | ||
| `HydroEnergyOutput` auxiliary variable. |
Collaborator
There was a problem hiding this comment.
Might be worth linking to the formulation library: formulas there clarify these things.
|
|
||
| ######################################### | ||
| ######## Loss-curve unit handling ####### | ||
| ######################################### |
Collaborator
There was a problem hiding this comment.
I suspect we already have helpers that do this sort of thing---look in IOM
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
2 participants
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.
Continues the pending work recorded in
.claude/pending-work.mdafter the events port merged in #286.lk/events-portwas already an ancestor ofmain, so there was nothing to rebase; this branch carries the follow-ups plus four workstreams.What's here
Loss-curve adoption (the CI/docs blocker). IS4 absorbed
lk/loss-curve-units-v2, soget_loss,get_loss_functionandget_converter_loss_from/_toreturn aLossCurvewrapper.get_proportional_term/get_constant_termare defined on the inner curve types only, so every reader needed unwrapping — twelve sites, including the twoHVDCVSCConverterPowerConstraintmethods thatpending-work.mddidn't list. Curve-shape branching moved fromisato dispatch.Two things worth reviewer attention:
_get_quadratic_term's untyped= 0.0fallback was a silent-failure hazard: aLossCurve-wrappedQuadraticCurvehit it and returned zero, dropping thea·I²term and disarming the guard that exists to refuse quadratic curves underLinearLossConverter. Demonstrated: aQuadraticCurve(2.5, 1.0, 0.5)returned2.5bare and0.0wrapped._loss_curve_valuereplaces the "assume the curve is authored in device base"base_factor. It reproduces the old arithmetic exactly for aDeviceBaseUnitcurve and corrects it for theNaturalUnitcurves the data actually carries, which the old code scaled as if per-unit. That is a deliberate numerics change — please sanity-check it against expected converter losses.EnergyLimitFeedforward— the storage twin ofReservoirLimitFeedforward, via a shared_add_integral_limit_constraints!. The two testsets fenced intest_storage_device_models.jlare live again; they had been written againstBookKeeping/BatteryAncillaryServices, which POM doesn't have, so they run againstStorageDispatchWithReserves. Themoi_testscounts were re-derived, not carried over.Source-conflict guard → trait. The four-member concrete-type
Unionwould have grown to five. The property cuts across the hierarchy (WaterLevelBudgetFeedforwarddeliberately doesn't opt in), so a shared supertype would be the wrong tool.Module-independent container keys.
metastrings interpolated aDataTypedirectly, which renders module-qualified when the name isn't visible in the active module — and the test runner uses a fresh module per file, so these keys were latently unstable.nameoffixes it, but src, the slack lookup, and the test expectations that rebuild the same keys all had to move in lockstep.Docs — the formulation library tabulated 5 of 10 feedforward types; all 10 now have a row and a guidance bullet.
Verification
mainbaselineThree files differ from baseline, all accounted for:
test_storage_device_models+12/+12 (the new tests),test_services_constructor±1 (baseline's own PSB cache race), andtest_events−4 in the parallel run but 68/8/19/95 in isolation, identical to baseline — the PSBforce_buildrace frompending-work.mditem 3, which hit 4× in that run against 1× in baseline.Clean compile,
detect_ambiguitiesempty, formatter clean.Why CI will be red — and it isn't this branch
IOMmaincalls a 5-arg unit-system-awareIS.convert_cost_coefficient; current IS4 defines only(value, ratio[, exponent]). IS commit2a2494ec("Move unit-system base arithmetic out of IS; take a ratio instead") — part of the samelk/loss-curve-units-v2merge that made this branch necessary — deliberately pushed that arithmetic into the domain package, and IOM hasn't caught up.Evidence it's external:
IS+IOMalonemainsourcegit diffbetween IOMmainand the previousjd/share-template-referencespin oncomponent_utils.jlis empty, so the pin change here didn't cause itIt's why the suite runs ~14k assertions against a recorded ~129k: models can't build past a thermal cost curve. This is the real gate on merging.
_loss_curve_ratio_to_system_basein this PR is a working reference for the fix — same three unit-system markers, resolving the ratio in the layer that owns the bases, exactly as IS's docstring now prescribes.Known-remaining debt (deliberately not in this PR)
metapattern survives instorage_models.jl(~1127/1134/1186/1216/1278) andhybrid_system_models/hybrid_systems.jl(~2066/2123). Write and read sides are self-consistent there, so it's latent, not broken — but it needs its own write/read audit rather than a bolt-on.hybrid_systems.jlalso hand-rolls what_service_container_metaalready computes.LossCurvefield shapes; HVDC test fixtures construct bareLinearCurve(...)where aLossCurve(...)wrapper is now required.Opened as a draft since CI cannot pass until IOM is fixed.