Refactor MBC/IEC for PSY static/time-series type split#74
Refactor MBC/IEC for PSY static/time-series type split#74luke-kiernan merged 21 commits intomainfrom
Conversation
|
Performance Results This branch |
There was a problem hiding this comment.
Pull request overview
Refactors offer-curve cost handling to align with PowerSystems’ split between static vs time-series cost types, replacing runtime “is time variant?” checks with type-based dispatch and updating tests accordingly.
Changes:
- Introduces union aliases for static/TS offer-curve cost types and updates offer-curve accessors, validation, and PWL data extraction to dispatch by cost type.
- Refactors startup/shutdown objective construction to use a trait (
_is_time_series_cost) instead of runtime checks, and updates mocks to mirror the static/TS type split. - Removes legacy test utilities and adds new integration tests covering static vs TS MarketBid/ImportExport code paths.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/definitions.jl | Adds union aliases for static + TS offer-curve cost types. |
| src/objective_function/value_curve_cost.jl | Updates offer-curve getters, time-series parameter detection, validation, and PWL data retrieval for new cost types. |
| src/objective_function/start_up_shut_down.jl | Uses trait-based dispatch for TS vs static startup/shutdown costs. |
| src/utils/powersystems_utils.jl | Extends is_time_variant to recognize TS-backed curve/value-curve types. |
| test/mocks/mock_components.jl | Splits mock operation costs into static vs TS variants and updates mock device typing. |
| test/test_start_up_shut_down.jl | Updates startup/shutdown tests to exercise the new trait-based TS path via mock TS cost type. |
| test/test_offer_curve_cost.jl | Adds integration tests for static vs TS MarketBid/ImportExport offer-curve code paths. |
| test/includes.jl | Removes includes for deleted legacy test utilities. |
| test/InfrastructureOptimizationModelsTests.jl | Registers the new integration test file. |
| test/test_utils/add_market_bid_cost.jl | Deleted legacy MBC test utility (no longer used). |
| test/test_utils/mbc_system_utils.jl | Deleted legacy MBC system utility (no longer used). |
| test/test_utils/iec_simulation_utils.jl | Deleted legacy IEC simulation utility (no longer used). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| breakpoint_param_container = get_parameter(container, BreakpointParam, T) | ||
| breakpoint_param_arr = get_parameter_column_refs(breakpoint_param_container, name) | ||
| breakpoint_param_mult = get_multiplier_array(breakpoint_param_container) | ||
| @assert size(breakpoint_param_arr) == size(breakpoint_param_mult[name, :, :]) | ||
| breakpoint_cost_component = | ||
| breakpoint_param_arr[:, time] .* breakpoint_param_mult[name, :, time] |
There was a problem hiding this comment.
In _get_raw_pwl_data (TS-backed curves), the breakpoint retrieval uses get_parameter(...); get_parameter_column_refs(..., name) and then indexes as if the result is 2D. For ObjectiveFunctionParameter containers, get_parameter_column_refs does not slice by device name (it only does that for TimeSeriesAttributes), so breakpoint_param_arr will remain 3D (name × point × time) and the subsequent size(...) assertion / indexing will fail. Use get_parameter_array(container, BreakpointParam, T) + get_parameter_multiplier_array(...) and index [name, :, time] (mirroring the slope path) to reliably extract breakpoint values.
| breakpoint_param_container = get_parameter(container, BreakpointParam, T) | |
| breakpoint_param_arr = get_parameter_column_refs(breakpoint_param_container, name) | |
| breakpoint_param_mult = get_multiplier_array(breakpoint_param_container) | |
| @assert size(breakpoint_param_arr) == size(breakpoint_param_mult[name, :, :]) | |
| breakpoint_cost_component = | |
| breakpoint_param_arr[:, time] .* breakpoint_param_mult[name, :, time] | |
| breakpoint_param_arr = get_parameter_array(container, BreakpointParam, T) | |
| breakpoint_param_mult = get_parameter_multiplier_array(container, BreakpointParam, T) | |
| @assert size(breakpoint_param_arr) == size(breakpoint_param_mult) | |
| breakpoint_cost_component = | |
| breakpoint_param_arr[name, :, time] .* breakpoint_param_mult[name, :, time] |
| elseif !(startup isa Float64) | ||
| throw( | ||
| ArgumentError( | ||
| "Expected Float64 or StartUpStages startup cost but got $(typeof(startup)) for $(get_name(device))", |
There was a problem hiding this comment.
validate_occ_component(::StartupCostParameter, ...) accepts NTuple{3, Float64} (and StartUpStages) as valid multi-start startup costs, but the thrown error message only mentions Float64/StartUpStages. Update the message to include NTuple{3, Float64} (or remove it from the accepted types) so the error is accurate for users.
| "Expected Float64 or StartUpStages startup cost but got $(typeof(startup)) for $(get_name(device))", | |
| "Expected Float64, NTuple{3, Float64}, or StartUpStages startup cost but got $(typeof(startup)) for $(get_name(device))", |
| active_power_limits::NamedTuple{(:min, :max), Tuple{Float64, Float64}} | ||
| base_power::Float64 | ||
| operation_cost::MockOperationCost | ||
| operation_cost::MockOperationCostTypes | ||
| must_run::Bool |
There was a problem hiding this comment.
MockThermalGen.operation_cost is now a union that can be MockTimeSeriesOperationCost, but IS.get_fuel_cost(g::MockThermalGen) = g.operation_cost.fuel_cost will throw when the TS cost variant is used (it has no fuel_cost field). Consider adding an overload for IS.get_fuel_cost(::MockThermalGen) that handles both variants (e.g., return 0.0 for MockTimeSeriesOperationCost or store fuel cost separately).
Adapt IOM to handle PSY's split of MarketBidCost → MarketBidCost + MarketBidTimeSeriesCost, and ImportExportCost → ImportExportCost + ImportExportTimeSeriesCost. Core changes: - Add MBC_TYPES/IEC_TYPES/TS_OFFER_CURVE_COST_TYPES union aliases - Add accessor overloads for TS cost types - Replace removed PSY.get_incremental_initial_input with curve traversal - Simplify _has_parameter_time_series to dispatch on cost type - Split _get_pwl_data into dispatched _get_raw_pwl_data (static vs TS) - Split validate_occ_breakpoints_slopes into dispatched _validate_occ_curves - Remove validate_initial_input_time_series (type guarantees consistency) - Add is_time_variant overloads for IS ValueCurve/CostCurve types - Add _shutdown_cost_value for LinearCurve → Float64 extraction Test utilities updated for new PSY types (LinearCurve fields, MarketBidTimeSeriesCost/ImportExportTimeSeriesCost construction). New integration tests for offer curve cost code paths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Static MarketBidCost.shut_down is now LinearCurve (was Float64). Extract the proportional term for the static shutdown cost path. ThermalGenerationCost.shut_down remains Float64. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t types Replace is_time_variant runtime checks in startup/shutdown with _is_time_series_cost trait dispatched at the function barrier. Julia specializes on the op_cost type parameter, so the branch is resolved at compile time for concrete cost types. Split MockOperationCost into static MockOperationCost (Float64 fields) and MockTimeSeriesOperationCost (signals "read from parameters"). The mock TS type opts into the TS path via _is_time_series_cost trait, avoiding coupling to PSY types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove mbc_system_utils.jl, add_market_bid_cost.jl, iec_simulation_utils.jl
(POM test utilities, not called within IOM)
- Make is_time_variant compile-time: dispatch on ValueCurve{<:TimeSeriesFunctionData}
type parameter instead of calling IS.is_time_series_backed at runtime
- Type op_cost as IS.DeviceParameter in startup/shutdown function signatures
- Mock costs subtype IS.DeviceParameter to match PSY hierarchy
- Consolidate CostAtMin validate_occ_component to single AbstractCostAtMinParameter method
- Pass (Type, name) instead of component instance in _get_raw_pwl_data
- Format
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
luke-kiernan
force-pushed
the
lk/mbc_iec_refactor
branch
from
177c65f to
b5595e8
Compare
|
@luke-kiernan we should merge #71 first |
- _get_raw_pwl_data (TS path): stale 2D/3D assertion and hardcoded NATURAL_UNITS; now reads power_units from the cost curve. - is_nontrivial_offer: shared predicate to replace dead isnothing(get_input_offer_curves(...)) checks (static MBC's default is ZERO_OFFER_CURVE, never nothing). - _add_start_up_cost_to_objective!: broadcast param * mult so Tuple-valued (multi-start) startup costs round-trip correctly. - is_time_variant_term: collapse to single-arg form (output depends only on cost type). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Lets POM's Renewable/Storage startup-nonzero validation traverse a TupleTimeSeries-backed `start_up` field on MarketBidTimeSeriesCost by delegating to the underlying TimeSeriesKey lookup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
# Conflicts: # src/common_models/interfaces.jl # src/objective_function/proportional.jl # src/objective_function/start_up_shut_down.jl # src/objective_function/value_curve_cost.jl # test/test_proportional.jl
|
Okay this is done. Tests pass locally, even if they don't pass in CI. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -625,8 +588,12 @@ function add_variable_cost_to_objective!( | |||
| ) where {T <: VariableType, U <: AbstractDeviceFormulation} | |||
| component_name = PSY.get_name(component) | |||
| @debug "Market Bid" _group = LOG_GROUP_COST_FUNCTIONS component_name | |||
| if !isnothing(get_input_offer_curves(cost_function)) | |||
| error("Component $(component_name) is not allowed to participate as a demand.") | |||
| if is_nontrivial_offer(get_input_offer_curves(cost_function)) | |||
| throw( | |||
| ArgumentError( | |||
| "Component $(component_name) is not allowed to participate as a demand.", | |||
| ), | |||
| ) | |||
There was a problem hiding this comment.
is_nontrivial_offer(::PSY.CostCurve{IS.TimeSeriesPiecewiseIncrementalCurve}) = false effectively disables the “no demand participation” guard for time-series-backed decremental offers. A device with a non-trivial TS decremental curve would no longer throw and could be treated as both supply and demand, changing objective/constraint behavior. Consider implementing a real non-triviality check for TS curves (e.g., read breakpoint/slope values from the parameter arrays for at least one time step, or compare against PSY’s ZERO_OFFER_CURVE placeholder if available) rather than hard-coding false.
| export get_parameter_multiplier_array, get_aux_variable, get_condition | ||
| export supports_milp, get_quadratic_cost_per_system_unit | ||
| export check_hvdc_line_limits_unidirectional, check_hvdc_line_limits_consistency | ||
| export add_sparse_pwl_interpolation_variables! |
There was a problem hiding this comment.
This PR removes previously exported public APIs (validate_available_devices, check_hvdc_line_limits_unidirectional, check_hvdc_line_limits_consistency). Even if unused internally, dropping exports is a breaking change for downstream packages relying on IOM’s public surface. If removal is intentional, it should be called out explicitly (and potentially gated by a major version bump); otherwise consider keeping deprecated forwarding methods/exports for at least one release cycle.
jd-lara
left a comment
jd-lara
left a comment
There was a problem hiding this comment.
I made several changes, the overarching change needed is to not rely on PSY and PSB for testing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| get_initial_input(::DecrementalOffer, device::PSY.StaticInjection) = | ||
| PSY.get_decremental_initial_input(PSY.get_operation_cost(device)) | ||
| IS.get_initial_input( | ||
| PSY.get_value_curve(get_input_offer_curves(PSY.get_operation_cost(device))), | ||
| ) |
There was a problem hiding this comment.
get_initial_input(::DecrementalOffer, device) now assumes get_input_offer_curves(get_operation_cost(device)) is always a cost curve and calls PSY.get_value_curve(...). Previously this code path tolerated missing/unused decremental curves (e.g. generators) via PSY.get_decremental_initial_input(...) / isnothing(...). If get_decremental_offer_curves can be nothing (or another placeholder), this will throw a MethodError. Consider explicitly handling an absent decremental curve (return nothing or a well-defined default) before calling get_value_curve.
| _get_parameter_field(::Type{<:DecrementalCostAtMinParameter}, op_cost) = | ||
| PSY.get_decremental_initial_input(op_cost) | ||
| IS.get_initial_input(PSY.get_value_curve(get_input_offer_curves(op_cost))) |
There was a problem hiding this comment.
_get_parameter_field(::Type{<:DecrementalCostAtMinParameter}, op_cost) calls get_input_offer_curves(op_cost) and immediately dereferences get_value_curve(...). If the decremental offer curve is optional for some cost types / devices, this will throw when it's unset. Suggest guarding for nothing/unused curves here as well, or delegating to a PSY getter that already encodes the default behavior.
| function is_nontrivial_offer(curve::PSY.CostCurve{PSY.PiecewiseIncrementalCurve}) | ||
| xs = PSY.get_x_coords(PSY.get_function_data(PSY.get_value_curve(curve))) | ||
| return last(xs) > first(xs) | ||
| end | ||
| is_nontrivial_offer(::PSY.CostCurve{IS.TimeSeriesPiecewiseIncrementalCurve}) = false | ||
|
|
There was a problem hiding this comment.
is_nontrivial_offer has no method for nothing (even though earlier logic used isnothing(get_input_offer_curves(...))) and hardcodes time-series curves to false. This can either (a) throw a MethodError when the decremental curve is absent, or (b) silently skip the demand-participation guard for MarketBidTimeSeriesCost even when a real decremental curve exists. Consider adding is_nontrivial_offer(::Nothing)=false and implementing a real check for TS curves (e.g., inspect breakpoints/slopes via the populated parameter arrays for a representative time step, or detect PSY's ZERO_OFFER_CURVE sentinel).
| ::Int, | ||
| ) = false | ||
| is_time_variant_term(::PSY.OperationalCost) = false | ||
|
|
There was a problem hiding this comment.
is_time_variant_term extension point signature was changed from the 6-argument form (container, op_cost, variable/device/formulation types, time) to a single-argument is_time_variant_term(::PSY.OperationalCost). This is a breaking change for downstream packages that may have implemented the old method, and it also removes the ability to decide time-variance based on container/time step. Consider keeping the old method as a fallback (possibly delegating to the new one) with a deprecation period, or providing both overloads.
| function is_time_variant_term( | |
| ::OptimizationContainer, | |
| op_cost::O, | |
| ::Type{V}, | |
| ::C, | |
| ::Type{F}, | |
| ::Int, | |
| ) where { | |
| O <: PSY.OperationalCost, | |
| V <: VariableType, | |
| C <: IS.InfrastructureSystemsComponent, | |
| F <: AbstractDeviceFormulation, | |
| } | |
| return is_time_variant_term(op_cost) | |
| end |
There was a problem hiding this comment.
There is an issue here, but not this one: see issue #83. FuelCurve still has fuel_cost::Union{Float64, TimeSeriesKey}: the proportional cost for thermal can be time-varying in a way that isn't apparent from the type of the operational cost.
There was a problem hiding this comment.
Here's a step towards a fix: IS #575. Working on a test case to confirm
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #74 +/- ##
==========================================
+ Coverage 30.47% 31.87% +1.39%
==========================================
Files 84 84
Lines 5240 5042 -198
==========================================
+ Hits 1597 1607 +10
+ Misses 3643 3435 -208
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| ::Int, | ||
| ) = false | ||
| is_time_variant_term(::PSY.OperationalCost) = false | ||
|
|
There was a problem hiding this comment.
There is an issue here, but not this one: see issue #83. FuelCurve still has fuel_cost::Union{Float64, TimeSeriesKey}: the proportional cost for thermal can be time-varying in a way that isn't apparent from the type of the operational cost.
We need to get the compile optimization once the code is correctly split without PSY dependency |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 53 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (3)
src/utils/component_utils.jl:58
- For
NetworkModeldispatch,get_available_componentscurrently forwards toIS.get_components(T, sys.data; ...), which assumes everyIS.InfrastructureSystemsContainerhas a.datafield and thatIS.get_componentsaccepts that internal object. This undermines the container interface and breaks duck-typed systems; prefer callingIS.get_components(T, sys; ...)(or add an explicit accessor/extension point to obtain the internal data safely).
src/quadratic_approximations/incremental.jl:213 - The DCVoltage special case is now inconsistent:
x_varis retrieved asget_variable(container, R, W)(keyed by device names), but whenR <: DCVoltageyou index it withx_name = get_name(get_dc_bus(d))(a bus name). This will fail unless the DCVoltage variable container is keyed by bus names and stored underW. Restore the previous dispatch (retrievex_varfrom the DC-bus component type) or make the x-variable container/keying consistent withx_name.
test/InfrastructureOptimizationModelsTests.jl:49 - The PR description states that 46 PSY-backed integration tests were added and that IOM adapts to PSY's new cost type split, but this diff removes integration-test gating and drops
PowerSystems/PowerSystemCaseBuilderfrom the test project. Either the description is outdated or these changes belong in a different PR; please align the PR description with the actual changes (or restore the integration test additions mentioned).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| horizon = interval = resolution = get_resolution(settings) | ||
| base_power = PSY.get_base_power(system) | ||
| sys_uuid = IS.get_uuid(system) | ||
| base_power = get_base_power(system) | ||
| # FIXME declare as stub | ||
| sys_uuid = IS.get_uuid(system.data.internal) | ||
| set_store_params!( |
There was a problem hiding this comment.
init_model_store_params! derives sys_uuid via IS.get_uuid(system.data.internal), which relies on internal fields that are not part of the IS.InfrastructureSystemsContainer interface. Prefer get_uuid(system) (or an explicit extension point) to avoid coupling to the .data.internal layout.
| @@ -302,17 +299,17 @@ function init_optimization_container!( | |||
| network_model::NetworkModel{T}, | |||
| sys::IS.InfrastructureSystemsContainer, | |||
| ) where {T <: AbstractPowerModel} | |||
| # PSY.set_units_base_system!(sys, "SYSTEM_BASE") | |||
| # set_units_base_system!(sys, "SYSTEM_BASE") | |||
| temp_set_units_base_system!(sys, "SYSTEM_BASE") | |||
| # The order of operations matter | |||
| settings = get_settings(container) | |||
|
|
|||
| if get_initial_time(settings) == UNSET_INI_TIME | |||
| if get_default_time_series_type(container) <: PSY.AbstractDeterministic | |||
| # set_initial_time!(settings, PSY.get_forecast_initial_timestamp(sys)) | |||
| if get_default_time_series_type(container) <: IS.AbstractDeterministic | |||
| # set_initial_time!(settings, IS.get_forecast_initial_timestamp(sys)) | |||
| set_initial_time!(settings, temp_get_forecast_initial_timestamp(sys)) | |||
There was a problem hiding this comment.
temp_get_forecast_initial_timestamp defaults to DateTime(1970). If a downstream package forgets to extend this for real systems, models will silently be built with an incorrect initial time (hard to detect, potentially corrupting results). Consider making the default throw a clear error (or require initial_time to be set) so misconfiguration fails fast.
There was a problem hiding this comment.
Agreed. Easiest fix that comes to mind: replace DateTime(1970) with a global constant sentinel and add checks in POM that error/warn if initial_time matches that sentinel. I've done the IOM half of that fix.
| function load_system(res::OptimizationProblemOutputs; kwargs...) | ||
| !isnothing(get_source_data(res)) && return | ||
| file = joinpath(get_outputs_dir(res), make_system_filename(get_source_data_uuid(res))) | ||
| if isfile(file) | ||
| sys = PSY.System(file; time_series_read_only = true) | ||
| sys = IS.InfrastructureSystemsContainer(file; time_series_read_only = true) | ||
| @info "De-serialized the system from files." |
There was a problem hiding this comment.
load_system attempts to deserialize via IS.InfrastructureSystemsContainer(file; ...). InfrastructureSystemsContainer is an abstract type/interface, so this call will not work unless InfrastructureSystems defines a callable function with that name (unlikely). This should be an extension point (implemented by downstream packages that know the concrete system type), or it should call the appropriate concrete constructor/deserialization API.
|
|
||
| # FIXME get_uuid declare as stub | ||
| make_system_filename(sys::IS.InfrastructureSystemsContainer) = | ||
| make_system_filename(IS.get_uuid(sys.data.internal)) |
There was a problem hiding this comment.
make_system_filename(sys::IS.InfrastructureSystemsContainer) reaches into sys.data.internal to get the UUID. Since get_uuid is imported and meant to be part of the public interface, prefer make_system_filename(get_uuid(sys)) to avoid depending on internal field layout.
| make_system_filename(IS.get_uuid(sys.data.internal)) | |
| make_system_filename(IS.get_uuid(sys)) |
| interval = IS.get_forecast_interval(system.data) | ||
| resolution = get_resolution(model) | ||
| base_power = PSY.get_base_power(system) | ||
| sys_uuid = IS.get_uuid(system) | ||
| base_power = get_base_power(system) | ||
| # FIXME declare as stub | ||
| sys_uuid = IS.get_uuid(system.data.internal) | ||
| store_params = ModelStoreParams( |
There was a problem hiding this comment.
init_model_store_params! hardcodes UUID extraction via IS.get_uuid(system.data.internal), which assumes the concrete system has .data.internal. Prefer get_uuid(system) (already imported from InfrastructureSystems) or an IOM-level extension point, so any IS.InfrastructureSystemsContainer implementation can work without relying on internal fields.
|
@luke-kiernan I fixed the conflicts. Check the status of the branch and this is ok, go ahead with the merge |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 53 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
src/operation/decision_model.jl:1
existing_typesis aSet{String}, so checkingSet([..]) ∈ existing_typescan never be true (the element types don’t match). This condition won’t detect mixed presence ofDeterministicandDeterministicSingleTimeSeries. Replace this with a check like(\"Deterministic\" in existing_types) && (\"DeterministicSingleTimeSeries\" in existing_types)(orissubset) so the error triggers correctly.
src/operation/problem_outputs.jl:1get_uuidis called unqualified here, but this PR removesPowerSystemsand does not add an import or stub forget_uuidinsrc/InfrastructureOptimizationModels.jl. This is likely anUndefVarErrorat runtime/compile time. Either qualify the call asIS.get_uuid(...)(using whatever object actually owns the UUID, e.g.sys.data.internal), or add/importaget_uuidextension-point function in the module and implement it for supported system containers.
src/operation/decision_model.jl:1- This now always passes
interval = _to_is_interval(model_interval)intoIS.get_forecast_horizon, even whenmodel_interval == UNSET_INTERVAL. The previous implementation only passedintervalwhen explicitly set, which avoids propagating a sentinel value into IS APIs. Restore the conditional kwarg behavior (only passintervalwhen notUNSET_INTERVAL) to prevent invalid interval queries.
test/InfrastructureOptimizationModelsTests.jl:1 - The PR description states that integration tests were added ("46 integration tests using real PSY systems"), but this change removes the integration-test gating/loading along with PSY/PSB dependencies in
test/Project.toml, and also removes PSY-backed test utilities. If integration tests are intended to remain, the description should be updated; otherwise, consider preserving an optional integration-test path behind an env flag (in a separate package or test target) so the stated test plan matches what the repository actually runs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| !isnothing(get_source_data(res)) && return | ||
| file = joinpath(get_outputs_dir(res), make_system_filename(get_source_data_uuid(res))) | ||
| if isfile(file) | ||
| sys = PSY.System(file; time_series_read_only = true) | ||
| sys = IS.InfrastructureSystemsContainer(file; time_series_read_only = true) | ||
| @info "De-serialized the system from files." | ||
| else | ||
| error("Could not locate system file: $file") |
There was a problem hiding this comment.
IS.InfrastructureSystemsContainer is a type, and (in typical IS/PSY patterns) not a deserialization constructor. This line is very likely to throw a MethodError. Use the concrete type that can be read from disk (e.g., a System/SystemData-backed constructor) or introduce an explicit deserialization hook/extension point so downstream packages (POM/PSY) can supply the correct loader.
| # alternatively, declare as stub and implement in POM. | ||
| if time_series_cache_size > 0 && IS.stores_time_series_in_memory(sys.data) | ||
| @info "Overriding time_series_cache_size because time series is stored in memory" | ||
| time_series_cache_size = 0 | ||
| end |
There was a problem hiding this comment.
This hard-requires sys to have a .data field with an IS-compatible storage backend, even though the signature is generic over system containers. Elsewhere in the PR you introduce stubs/extension points for container-specific accessors; using one here (e.g., stores_time_series_in_memory(sys) via dispatch) would keep Settings compatible with any IS.InfrastructureSystemsContainer implementation and avoid leaking internal representation (.data) into IOM’s public surface.
There was a problem hiding this comment.
We need to flag this in an issue to follow up with a solution that doesn't rely on accessing the data field
| device_names = IS.get_name.(devices) | ||
| add_dual_container!(container, constraint_type, D, device_names, time_steps) | ||
| else | ||
| # Reuse the existing constraint container's row axis so the dual axis |
There was a problem hiding this comment.
Do we have tests for this somewhere (probably need to go in POM)? If not, open an issue to add them.
| @@ -302,17 +299,17 @@ function init_optimization_container!( | |||
| network_model::NetworkModel{T}, | |||
| sys::IS.InfrastructureSystemsContainer, | |||
| ) where {T <: AbstractPowerModel} | |||
| # PSY.set_units_base_system!(sys, "SYSTEM_BASE") | |||
| # set_units_base_system!(sys, "SYSTEM_BASE") | |||
| temp_set_units_base_system!(sys, "SYSTEM_BASE") | |||
| # The order of operations matter | |||
| settings = get_settings(container) | |||
|
|
|||
| if get_initial_time(settings) == UNSET_INI_TIME | |||
| if get_default_time_series_type(container) <: PSY.AbstractDeterministic | |||
| # set_initial_time!(settings, PSY.get_forecast_initial_timestamp(sys)) | |||
| if get_default_time_series_type(container) <: IS.AbstractDeterministic | |||
| # set_initial_time!(settings, IS.get_forecast_initial_timestamp(sys)) | |||
| set_initial_time!(settings, temp_get_forecast_initial_timestamp(sys)) | |||
There was a problem hiding this comment.
Agreed. Easiest fix that comes to mind: replace DateTime(1970) with a global constant sentinel and add checks in POM that error/warn if initial_time matches that sentinel. I've done the IOM half of that fix.
| # alternatively, declare as stub and implement in POM. | ||
| if time_series_cache_size > 0 && IS.stores_time_series_in_memory(sys.data) | ||
| @info "Overriding time_series_cache_size because time series is stored in memory" | ||
| time_series_cache_size = 0 | ||
| end |
Summary
MarketBidCost→MarketBidCost+MarketBidTimeSeriesCost, andImportExportCost→ImportExportCost+ImportExportTimeSeriesCostUnion{Float64, TimeSeriesKey}) — the cost type now determines whether data is static or time-series-backedis_time_variantfield checks with compile-time type dispatch where possible (_get_raw_pwl_data,_validate_occ_curves,_is_time_series_costtrait for startup/shutdown)MockOperationCost(static) andMockTimeSeriesOperationCost(TS), both<: IS.DeviceParametermbc_system_utils.jl,add_market_bid_cost.jl,iec_simulation_utils.jl) — not called within IOMTest plan
🤖 Generated with Claude Code