Iterate DeviceModel's device_cache, and narrow the device-collection signatures to Vector - #307
luke-kiernan wants to merge 9 commits into
Conversation
`test/Project.toml` pinned InfrastructureOptimizationModels to `rh/cost_coefficient_ratio`, the branch behind IOM #166. That PR merged and the branch was deleted, so the environment no longer resolves at all: ERROR: Did not find rev rh/cost_coefficient_ratio in repository The root `Project.toml` was already on `rev = "main"`; only the test environment still carried the branch pin. Repoint it and drop the comment explaining the temporary pin, which #166 landing has made obsolete. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rator
Closes the POM half of InfrastructureOptimizationModels#79. `DeviceModel`
now caches its components in a concretely-typed `device_cache::Vector{D}`,
filled by `make_device_cache!` during template validation. The constructors
still called `get_available_components(model, sys)`, rebuilding a
`FlattenIteratorWrapper` on every construct stage: a wrapper over
`Iterators.Flatten` of a generator of dict values, whose nested-tuple iterate
state inference struggles with, and which offers no `getindex`, `IndexStyle`
or `HasShape`. Swap those ~200 call sites for `get_device_cache(model)`.
IOM#157 widened IOM's own device-collection signatures for this, but POM has
~320 helpers of its own still hard-typed to the iterator, so a `Vector`
reached them as a MethodError:
no method matching _handle_common_thermal_parameters!(
::OptimizationContainer, ::Vector{ThermalStandard}, ::DeviceModel{...})
Widen those to `Union{Vector{T}, IS.FlattenIteratorWrapper{T}}`, the
convention 155 signatures here already follow. Annotations only: every body
is a `for d in devices` or a comprehension that works unmodified on a Vector.
Three sites clone a DeviceModel outside `validate_template!` and so never get
a cache; each now carries one over explicitly, or it silently constructs
against zero devices:
- `initialization.jl` builds the IC template from
`get_initial_conditions_device_model` clones and calls `build_problem!`
directly, bypassing validation entirely.
- `load_constructor.jl` rebuilds a controllable-load model as
`StaticPowerLoad` and recurses into `construct_device!`.
- `mock_operation_models.jl` (test harness) constructs devices without a
template at all.
`validate_available_devices` deliberately keeps using
`get_available_components`: it runs once per DeviceModel rather than per
device, so it gains nothing from the cache, and it is called directly on
freshly built models that have none.
Verified against a full-suite baseline on main: all 54 test files report
identical Fail/Error counts with and without this change. The 16 files that
are red are red on main too, from the `ramp_limits` units error that #304
fixes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jd-lara
left a comment
There was a problem hiding this comment.
Can we just make all the inputs vector{} I think we can remove iterator from the functions signatures
Claude code says it's doable, requires adding |
We should check the places where the collect is needed and we probably don't really do need it, it could be missing caches |
With the constructors passing `DeviceModel`'s `device_cache`, the
`Union{Vector{T}, IS.FlattenIteratorWrapper{T}}` annotations no longer earn
their width: every device collection reaching a builder is a `Vector`. Narrow
all of them, leaving one convention instead of two.
This is not a performance change. Julia specializes on the concrete argument
type at each call site regardless of how wide the annotation is, so the Union
compiled exactly as well; the speed came from passing the cache. What this
buys is a narrower contract and a single spelling.
`_make_device_cache` keeps `IS.FlattenIteratorWrapper{T}`: it is the boundary
where the iterator becomes a Vector, so it is the one place the type belongs.
Five sources still produced an iterator and now materialize at the source:
- `make_system_expressions.jl` areas, for the area-based network models.
- The AGC service constructors (services and areas alike).
- `get_available_reservoirs`, a bespoke helper in `utils/psy_utils.jl` that
feeds 12 hydro call sites and is invisible to a grep for
`get_available_components` or `PSY.get_components`.
- Tests calling builders directly with `PSY.get_components(...)`.
- `test_import_export_cost.jl`, likewise, via `add_constraints!`.
Two pre-existing `devices_vec = collect(devices)` calls in the hybrid models
were written when `devices` was an iterator; they are redundant copies now
that it arrives as the cache vector, and both uses are read-only, so they
alias instead. No `collect` in `src/` sits inside a loop.
One hazard worth recording: because IOM's methods still accept the Union, a
stray iterator does not fail at the boundary. It falls through to a more
generic method and surfaces far away -- as `get_fixed(::MarketBidCost)` deep
in the cost path, or as POM's own "add_parameters! not implemented for ...
Implement this method" fallback, which reads as a missing feature rather than
a dispatch miss. Narrowing IOM to match would restore a loud boundary.
Verified against a full-suite run of the previous commit in this same
environment: all 54 test files report identical Fail/Error counts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Performance Results
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
I think that test failures are unrelated but @luke-kiernan can you check them and fix the testing please? |
Three CI failures, all from this branch's own sweep meeting code that
merged from main afterwards:
- `_seed_range_expression!` (services_constructor.jl) was the one call
site the sweep missed; it arrived with the mb/reserve-bug-fix merge
written against `get_available_components`. Passing an iterator into
the narrowed `add_expressions!` surfaced far from the cause, as a
`DecisionModel Build Failed` and then a `KeyError` on a constraint
that was never built. It accounts for the storage, hybrid and
reserve-offer failures. `sys` is unused once the cache is passed, so
it drops out of the helper's signature.
- `electric_loads.jl` still declared `IS.FlattenIteratorWrapper` on
`add_constraints!` and `_add_interruption_gate!`; both came in with
the interruptible-load gate fix. Now `Vector{V}`, leaving
`_make_device_cache` as the only iterator-typed signature in src/.
- `_two_area_sys_with_lcc_tie` and `_two_area_sys_with_lossy_hvdc_tie`
kept the closing `),` of a `collect(` that d67f502 removed, so the
assignment parsed as a tuple that read `existing_arcs` before it was
bound.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docstring sat directly above `_countdown_at_step`, so Documenter bound it to the private helper and left `countdown_trajectory` (which is exported) undocumented, breaking the `@ref` to it in the `availability_trajectory` docstring. Pre-existing on main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lk/device-cache-iteration
Should be fixed now |
Closes the POM half of IOM #79. IOM #157 was the prep; this is the "real meat" that PR's description promised.
1. Pass the cache instead of rebuilding the iterator
DeviceModelcaches its components in a concretely-typeddevice_cache::Vector{D}, filled bymake_device_cache!during template validation. The constructors still calledget_available_components(model, sys)on every construct stage, rebuilding aFlattenIteratorWrapper— a wrapper overIterators.Flattenof a generator of dict values, whose nested-tuple iterate state inference struggles with, and which offers nogetindex,IndexStyleorHasShape. ~200 call sites now readget_device_cache(model).Clones that never see template validation
Three sites build a
DeviceModeloutsidevalidate_template!, so its cache is empty and construction silently proceeds against zero devices. Each now carries one over explicitly:initialization.jl— the IC template is assembled fromget_initial_conditions_device_modelclones and handed straight tobuild_problem!, bypassing validation entirely.load_constructor.jl— the controllable-load fallback rebuilds the model asStaticPowerLoadand recurses intoconstruct_device!.test_utils/mock_operation_models.jl— the mock harness constructs devices with no template at all.validate_available_devicesdeliberately keepsget_available_components: it runs once perDeviceModelrather than per device, so it gains nothing from the cache, and it is called directly on freshly built models that have none.2. Narrow the signatures to
VectorWith the cache being passed,
Union{Vector{T}, IS.FlattenIteratorWrapper{T}}no longer earns its width. All of them narrow toVector{T}, leaving one convention instead of two — including the 155 that already used the Union before this PR.This is not a performance change. Julia specializes on the concrete argument type at each call site regardless of annotation width, so the Union compiled exactly as well. The speed came from passing the cache. What narrowing buys is a tighter contract and a single spelling.
_make_device_cachekeepsIS.FlattenIteratorWrapper{T}— it is the boundary where the iterator becomes aVector, so it is the one place the type belongs.Five sources still produced an iterator and now materialize at the source: the
make_system_expressions.jlareas; the AGC service constructors;get_available_reservoirs(a bespoke helper inutils/psy_utils.jlfeeding 12 hydro call sites, invisible to a grep forget_available_componentsorPSY.get_components); and two sets of tests that call builders directly withPSY.get_components(...).Two pre-existing
devices_vec = collect(devices)calls in the hybrid models were written whendeviceswas an iterator. They are redundant copies now that it arrives as the cache vector, and both uses are read-only, so they alias instead. Nocollectinsrc/sits inside a loop.One hazard worth flagging for review
Because IOM's methods still accept the Union, a stray iterator does not fail at the boundary. It falls through to a more generic method and surfaces far away — as
get_fixed(::MarketBidCost)deep in the cost path, or as POM's ownadd_parameters! not implemented for ... Implement this method in PowerOperationsModelsfallback, which reads as a missing feature rather than a dispatch miss. Narrowing IOM to match would restore a loud boundary. Reviewers may reasonably prefer to split section 2 into a coordinated POM+IOM change.Testing
Full-suite baseline versus this branch, in the same environment: all 54 test files report identical Fail/Error counts, verified separately after section 1 and after section 2.
test_aquapasses, so no new method ambiguities.The 16 files that are red are red on
maintoo, from the pre-existingramp_limits requires a time on the units argumenterror that #304 fixes. CI will show them until that lands.Follow-up, not done here
The 12 hydro reservoir/turbine call sites go through
get_available_reservoirs(sys)rather thanget_device_cache(model), so IOM #79's goal is not yet met for hydro — they still materialize per construct stage. Worth its own issue.Also here
A separate commit repoints
test/Project.toml's IOM pin fromrh/cost_coefficient_ratiotomain. That branch was IOM #166, which merged and was deleted, so the test environment did not resolve at all onmain:🤖 Generated with Claude Code