Accept unit-tagged values in component constructors - #1759
Open
luke-kiernan wants to merge 3 commits into
Open
Conversation
A component can now be constructed with natural-unit or device-base values
while it is detached from a System:
ThermalStandard(; ..., active_power = 50.0u"MW", base_power = 100.0)
TransformerCircuit(; ..., r = 5.29u"Ω", base_power = 100.0,
base_voltage_primary = 230.0)
ThermalStandard(; ..., active_power = 0.5 * DU) # device base, tagged
ThermalStandard(; ..., active_power = 0.5) # untagged: still device base
Getters and setters resolve their per-unit bases from the component. A
constructor has no component yet -- and no System to fall back on -- so the
bases come from the constructor's own arguments. The generated constructors
collect them into a `_construction_fields` NamedTuple and call `construct_value`
once per unit-bearing field.
Generator (src/generate_structs.jl):
- `constructor_value` routes each `needs_conversion` parameter through
`construct_value`, resolved in the generating module exactly like the
`get_value`/`set_value` the accessors already emit.
- The kwarg constructor needs the same treatment, not just forwarding: it
passes `internal`, so it calls the default all-fields constructor rather
than the positional one. Emitting the conversion only in the positional
constructor would silently skip the kwarg path.
- Gated on a new per-item `has_conversions` flag, so structs with no
unit-bearing fields generate byte-identically to before.
Domain hook (src/models/components.jl):
- `UnderConstruction{T, F, P, V}` -- isbits carrier for one field's bases.
It joins `UnitsBearer`, so it is a first-class base provider for the
existing engine rather than a parallel mechanism. `T`/`F` are carried for
error messages only.
- `_construction_base` mirrors `_conversion_base`, reading constructor
arguments instead of component fields. The generic resolver walks
`base_power`, then `base_voltage_primary` -> `base_voltage` -> `arc.from`
-> `bus`. Overrides cover the transformers' `magnetizing_shunt` and the six
3W pairwise impedances.
- `_construct_value` delegates every tagged value to `set_value`, so the
conversion math is not duplicated -- there is still exactly one conversion
engine. It adds only what the setters cannot have: untagged `Real`/`Complex`
pass through as device base, and compound fields (`MinMax`, `UpDown`,
`FromTo`, `FromTo_ToFrom`, `StartUpShutDown`) recurse so untagged entries
inside them keep working.
- `base_power_kind` moves to the type level, since construction resolves the
trait before any component exists; the instance method forwards.
`TransformerCircuit` gets its own method -- it is a `DeviceParameter`, not a
`Component`, so the default did not reach it, and its `base_power` is a
genuine per-winding device base.
Untagged numbers keep meaning device base, so every existing call site is
unaffected. The setters reject bare floats; the constructors cannot, since that
would break essentially all existing construction. Making units mandatory here
is a separate, later step.
Two cases throw, with the field named and an actionable message:
1. `SU` values, always -- the system base is unknown before `add_component!`.
2. Natural units on `SystemBasePower` types (Line, MonitoredLine, Area, the
TwoTerminal* lines, ...). Their `base_power` field only records the system
base, and `add_component!` resyncs it on attachment via
`_sync_base_power!` -- so per-unitizing against the constructor's argument
could freeze a value against a base the system does not share. Device-base
values work for all of them, and the transformers' own data lives on
TransformerCircuit, which carries its bases and converts fine. Whether to
support this is an open question; it is isolated to a single
`_construction_base_power` method.
Contributor
Author
|
Tests are failing due to PSB not compiling. The same happens on |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends PowerSystems’ generated component constructors to accept unit-tagged (natural-unit) values while the component is not yet attached to a System, by resolving per-unit bases from the constructor arguments and reusing the existing getter/setter conversion engine.
Changes:
- Updates the struct generator to route unit-bearing constructor arguments through
construct_value, for both positional and keyword constructors (gated by a per-structhas_conversionsflag). - Adds an
UnderConstructionbase provider and constructor-time base-resolution logic insrc/models/components.jl, delegating conversion to existingset_valuemachinery. - Adds unit-aware construction tests and adjusts test dependency sourcing for
PowerSystemCaseBuilder.
Reviewed changes
Copilot reviewed 4 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_units.jl | Adds coverage for unit-tagged constructor arguments (including compound fields) and expected error cases. |
| test/Project.toml | Updates PowerSystemCaseBuilder source to a Git URL+rev for tests. |
| src/generate_structs.jl | Generator: introduces constructor_value and has_conversions to emit constructor-time conversions without affecting structs that don’t need them. |
| src/models/components.jl | Implements UnderConstruction base provider and construct_value conversion path for constructor arguments. |
| src/models/generated/TwoWindingTransformer.jl | Generated constructors now convert magnetizing_shunt via construct_value. |
| src/models/generated/TwoTerminalVSCLine.jl | Generated constructors now convert power-related fields via construct_value. |
| src/models/generated/TwoTerminalLCCLine.jl | Generated constructors now convert power-related fields via construct_value. |
| src/models/generated/TwoTerminalGenericHVDCLine.jl | Generated constructors now convert power-related fields via construct_value. |
| src/models/generated/TransmissionInterface.jl | Generated constructors now convert active_power_flow_limits via construct_value. |
| src/models/generated/TransformerCircuit.jl | Generated constructors now convert impedance/power fields via construct_value. |
| src/models/generated/TModelHVDCLine.jl | Generated constructors now convert active_power_flow/limit fields via construct_value. |
| src/models/generated/ThreeWindingTransformer.jl | Generated constructors now convert impedances and magnetizing_shunt via construct_value. |
| src/models/generated/ThermalStandard.jl | Generated constructors now convert power/ramp-related fields via construct_value. |
| src/models/generated/ThermalMultiStart.jl | Generated constructors now convert power/ramp/trajectory fields via construct_value. |
| src/models/generated/SynchronousCondenser.jl | Generated constructors now convert reactive power/rating/loss fields via construct_value. |
| src/models/generated/StandardLoad.jl | Generated constructors now convert load power components via construct_value. |
| src/models/generated/Source.jl | Generated constructors now convert power/limits fields via construct_value. |
| src/models/generated/ShiftablePowerLoad.jl | Generated constructors now convert power/limits/max fields via construct_value. |
| src/models/generated/RenewableNonDispatch.jl | Generated constructors now convert active/reactive/rating fields via construct_value. |
| src/models/generated/RenewableDispatch.jl | Generated constructors now convert active/reactive/rating/limits fields via construct_value. |
| src/models/generated/PowerLoad.jl | Generated constructors now convert active/reactive/max fields via construct_value. |
| src/models/generated/MotorLoad.jl | Generated constructors now convert active/reactive/rating/max/limits fields via construct_value. |
| src/models/generated/MonitoredLine.jl | Generated constructors now convert flow/impedance/admittance/rating fields via construct_value. |
| src/models/generated/LoadZone.jl | Generated constructors now convert peak power fields via construct_value. |
| src/models/generated/Line.jl | Generated constructors now convert flow/impedance/admittance/rating fields via construct_value. |
| src/models/generated/InterruptibleStandardLoad.jl | Generated constructors now convert load power components via construct_value. |
| src/models/generated/InterruptiblePowerLoad.jl | Generated constructors now convert active/reactive/max fields via construct_value. |
| src/models/generated/InterconnectingConverter.jl | Generated constructors now convert power/limits/current-related fields via construct_value. |
| src/models/generated/HydroTurbine.jl | Generated constructors now convert power/limits/ramp fields via construct_value. |
| src/models/generated/HydroPumpTurbine.jl | Generated constructors now convert power/limits/ramp/pump fields via construct_value. |
| src/models/generated/HydroDispatch.jl | Generated constructors now convert power/limits/ramp fields via construct_value. |
| src/models/generated/HybridSystem.jl | Generated constructors now convert power/limits/interconnection rating fields via construct_value. |
| src/models/generated/GenericArcImpedance.jl | Generated constructors now convert flow/max flow/impedance fields via construct_value. |
| src/models/generated/FACTSControlDevice.jl | Generated constructors now convert reactive/current limit fields via construct_value. |
| src/models/generated/ExponentialLoad.jl | Generated constructors now convert active/reactive/max fields via construct_value. |
| src/models/generated/EnergyReservoirStorage.jl | Generated constructors now convert capacity/rating/power/limits/ramp/loss fields via construct_value. |
| src/models/generated/DiscreteControlledACBranch.jl | Generated constructors now convert flow/impedance/rating fields via construct_value. |
| src/models/generated/AreaInterchange.jl | Generated constructors now convert flow/limits fields via construct_value. |
| src/models/generated/Area.jl | Generated constructors now convert peak power fields via construct_value. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+386
to
+398
| function _construction_base_voltage(f::NamedTuple) | ||
| if hasproperty(f, :base_voltage_primary) # TransformerCircuit | ||
| return f.base_voltage_primary | ||
| elseif hasproperty(f, :base_voltage) # buses | ||
| return f.base_voltage | ||
| elseif hasproperty(f, :arc) # branches | ||
| return get_base_voltage(f.arc.from) | ||
| elseif hasproperty(f, :bus) # injectors | ||
| return get_base_voltage(f.bus) | ||
| else | ||
| return nothing | ||
| end | ||
| end |
Contributor
Author
There was a problem hiding this comment.
Only Source and TransformerCircuit have a nullable base voltage and something to fall back to. And for Source all unit conversion fields are :mva when voltage is only needed for :ohms and :siemens. So right now, this consideration is only relevant for TransformerCircuit.
If the user declines to give base_voltage_primary when constructing a TransformerCircuit, should construction assume the from-bus's voltage or error?
Review flagged that `_construction_base_voltage` returns `base_voltage_primary`/ `base_voltage` whenever the argument exists, even when it is `nothing`, and suggested falling through to `arc.from`/`bus` in that case. Keying on presence is deliberate and must stay: this function has to mirror `get_base_voltage` exactly, or a value per-unitized at construction is read back against a different base. The two types that own a nullable voltage field *and* an `arc`/`bus` resolve it at runtime straight from that field, with no fallback: - `get_base_voltage(w::TransformerCircuit) = get_base_voltage_primary(w)` - the generated `get_base_voltage(value::Source) = value.base_voltage` So with `base_voltage_primary = nothing`, `set_r!(w, 5.29u"Ω")` already errors even though the arc's from-bus carries a base voltage. Falling through here would make construction *more* permissive than the setters: the constructor would silently store `r` per-unitized on the bus's voltage, and reading it back in Ω would then error. Erroring on both paths is the consistent behavior. (`Source` cannot exhibit this at all -- all four of its `needs_conversion` fields are `:mva`, so base voltage is never consulted for them.) No behavior change. Adds the comment recording the invariant, plus a regression test asserting that construction and `set_r!` refuse in exactly the same case, so a future edit does not "fix" this into the bug it looks like.
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.
Closes #1115. Replaces #1756, which was written before struct generation was forked into PSY and before the
BasePowerKindtrait landed; re-implemented on currentpsy6rather than rebased. The companion IS PR (Sienna-Platform/InfrastructureSystems.jl#606) is no longer needed — the generator lives here now, so this is a single-repo change and there are no[sources]pins to add or restore.A component can now be constructed with natural-unit or device-base values while it is detached from a
System:Design
Getters and setters resolve their per-unit bases from the component. A constructor has no component yet — and no
Systemto fall back on — so the bases come from the constructor's own arguments. The generated constructors collect them into a_construction_fieldsNamedTuple and callconstruct_valueonce per unit-bearing field.Generator (
src/generate_structs.jl):constructor_valueroutes eachneeds_conversionparameter throughconstruct_value, resolved in the generating module exactly like theget_value/set_valuethe accessors already emit.internal, so it calls the default all-fields constructor rather than the positional one — emitting the conversion only in the positional constructor would silently skip the kwarg path.has_conversionsflag, so structs with no unit-bearing fields generate byte-identically to before. 35 of ~210 files changed.Domain hook (
src/models/components.jl):UnderConstruction{T, F, P, V}— isbits carrier for one field's bases. It joinsUnitsBearer, so it is a first-class base provider for the existing engine rather than a parallel mechanism;T/Fare carried for error messages only._construction_basemirrors_conversion_base, reading constructor arguments instead of component fields. The generic resolver walksbase_power, thenbase_voltage_primary→base_voltage→arc.from→bus. Overrides cover the transformers'magnetizing_shunt(delegates to the already-formedTransformerCircuit, which is its own base provider) and the six 3W pairwise impedances (base_power_ijreferenced to the first-index circuit's voltage, matching the existingPairBaseconvention)._construct_valuedelegates every tagged value toset_value, so the conversion math is not duplicated — there is still exactly one conversion engine. It adds only what the setters cannot have: untaggedReal/Complexpass through as device base, and compound fields (MinMax,UpDown,FromTo,FromTo_ToFrom,StartUpShutDown) recurse so untagged entries inside them keep working.base_power_kindmoves to the type level, since construction resolves the trait before any component exists; the instance method forwards, soadd_component!and the setters are unaffected.TransformerCircuitgets its own method — it is aDeviceParameter, not aComponent, so the default did not reach it, and itsbase_poweris a genuine per-winding device base.Behavior notes
Untagged numbers keep meaning device base, so every existing call site is unaffected — including the OpenAPI importers, which already divide by
base_powerthemselves and pass bare device-base floats. (The setters reject bare floats; the constructors cannot, since that would break essentially all existing construction. Making units mandatory here is a separate, later step.)Two cases throw, with the field named and an actionable message:
SUvalues, always — the system base is unknown beforeadd_component!.Natural units on⚠️ This is the open design question. Those types now carry a
SystemBasePowertypes (Line,MonitoredLine,Area, theTwoTerminal*lines, …).base_powerfield, so the conversion is arithmetically available — but the field only records the system base, andadd_component!resyncs it on attachment via_sync_base_power!. Per-unitizing against the constructor's argument would freeze a value against a base the system may not share:Line(; r = 5.29u"Ω", base_power = 100.0)added to a 200 MVA system would silently leavermeaning something else. It errors rather than guessing. Device-base values work for all of them, and the transformers' own data lives onTransformerCircuit, which carries its bases and converts fine.Supporting it is a one-method deletion (
_construction_base_power(::SystemBasePower, ...)), flagged with a TODO. Happy to flip it if we decide the resync hazard is acceptable or should be handled some other way.Verification
New
Units-aware constructiontestset intest/test_units.jl, covering:MW/DU/untagged agreement; positional ≡ keyword; construction ≡ construct-then-set; compound fields with mixed tagging;TransformerCircuitfrom Ω while detached (5.29 Ω → 0.01 pu on 230 kV/100 MVA); and both error paths.Each of these was confirmed passing by running the construction paths directly against a build of this branch.
test/runtests.jlcannot load in my environment:PowerSystemCaseBuilderfails to precompile withUndefVarError: OpenAPISystem not defined in PowerTableDataParser, which is a checkout-version skew upstream of this change. CI is the real check here. Docs were not touched.🤖 Generated with Claude Code