Skip to content

Unitful getters and setters#1659

Merged
jd-lara merged 19 commits into
psy6from
lk/units-fold-psu
May 29, 2026
Merged

Unitful getters and setters#1659
jd-lara merged 19 commits into
psy6from
lk/units-fold-psu

Conversation

@luke-kiernan

@luke-kiernan luke-kiernan commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

Add unitful getters and setters. Interface:

get_active_power(gen) # errors: must specify units
get_active_power(gen, {NU/SU/DU}) # natural units/system base/device base, with units attached 
get_active_power(gen, Float64) # system base, no units attached: internal usage.

5 + get_active_power(gen, NU) # errors: unitless combined with unitful
5 MW + get_active_power(gen, NU) # ok
0.5 SU + get_active_power(gen, NU) # errors: relative combined with natural

set_active_power!(gen, 0.5) # errors: must specify units
set_active_power!(gen, 0.5 {SU/DU}) # system base/device base
set_active_power!(gen, 50 MW) # natural units

Relies on IS #574. This will break stuff downstream left and right, yes, but here's why I think it's worth it:

  1. Usability. It is very very easy to unknowingly mix units. The classic one: read in a time series from a CSV in natural units, then attach it to a system in SYSTEM_BASE.
  2. Type stability. Our stateful unit system is inherently type unstable.
  3. Maintenance. After the catch-all "not having adequate test cases," I've found more bugs due to units issues than anything else.

edit: at the moment, turns out 1-argument getters/setters assume system base. But I'd prefer to have them error: better that than get silently incorrect results. Also, I'd consider adding a 3-arg setter, with the units separate: set_active_power!(gen, 1, {DU/SU/NU}

edit: You can still mess up by adding or comparing device-base quantities from components with different base powers. Same goes for comparing SU quantities between different systems.

@luke-kiernan luke-kiernan requested a review from jd-lara April 17, 2026 17:13
@luke-kiernan

luke-kiernan commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

Here is perhaps the biggest argument for these changes:
units_performance_comparison
This is profiling "add up active powers on all thermal gens in the 10k bus system:"

function sum_su_strip(gens)
    s = 0.0
    for g in gens
        s += IS.ustrip(get_active_power(g, IS.SystemBaseUnit())) # change this line
    end
    return s
end

luke-kiernan and others added 8 commits April 24, 2026 17:02
- Vendor PSU sources into src/units/ (types, conversions, serialization).
  RelativeQuantity, AbstractRelativeUnit, DU/SU/NU, convert_units, and
  serialize_quantity now live in PSY.
- Replace stateful unit-system with explicit-units API on Component:
  get_value / set_value use Val-dispatch through _convert_from_device_base
  for type-stable conversions (DU/SU/MW/Mvar/Ω/S/Float64 targets).
- DEFAULT_UNITS = SU; Float64 fast path for hot loops returns raw
  system-base numbers without the wrapper.
- Add MW_ACCUMULATOR_TYPE for _sum_or_zero helpers in system_checks;
  total_capacity_rating / total_load_rating return MW-typed Unitful.Quantity.
- Fix convert_component! (PowerLoad → StandardLoad) to copy raw device-base
  fields instead of round-tripping through SU getters.
- Add supplemental 2-arg get_max_active_power overloads for StaticInjection
  and Union{StandardLoad, InterruptibleStandardLoad}.
- Drop PowerSystemsUnits dep; add Unitful and StructTypes deps.
- Port PSU test suite as test/test_units.jl (77 tests).
- Update tests for unit-bearing return types: test_accessors uses
  _unwrap_units helper; test_printing unwraps units for occursin check;
  with_units_base testsets replaced by explicit-units API tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated files now emit:
  get_<field>(value::T)        = get_value(value, Val(:<field>), Val(:<cu>))
  get_<field>(value::T, units) = get_value(value, Val(:<field>), Val(:<cu>), units)

and matching set_<field>!(value::T, val) lines, replacing the previous
stateful-unit-system accessors.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Template-generated 1-arg getters are gone; all unit-bearing getters now
  take an explicit units argument. Bare `set_foo!(c, 0.5)` errors rather
  than silently treating the value as system base.
- DU/SU/NU and RelativeQuantity now come from IS; this package imports
  and re-exports them, and trims src/units/types.jl to the power-specific
  Unitful @Units (Mvar, MVA) and the UnitArg alias.
- Supplemental, HybridSystem, DynamicBranch, and generation.jl getters
  migrated to 2-arg. print.jl `_show_accessor_value` uses the new
  IS.display_units_arg trait for display-units selection.
- Tests updated accordingly; relative-unit primitive tests moved to IS.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Regenerated against the updated IS template: unit-bearing structs now emit
only the 2-arg getter plus a `display_units_arg(::typeof(f), ::Type{T}) = SU`
line per field, keyed on both function and struct to avoid spooky
cross-type aliasing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Public get_base_power(c, units) dispatches on NU/MW/SU/DU/Float64. The
unitless accessor is now _get_base_power (used as the internal conversion
anchor). Same pattern for System and for ThreeWindingTransformer winding
base powers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Descriptor now marks base_power fields exclude_getter=true so the generator
emits an internal _get_ accessor; the public unit-aware getter is hand-written.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow the IS CostCurve/FuelCurve parameterization: add U<:AbstractUnitSystem
as a type parameter on ImportExportCost, MarketBidCost, ReserveDemandCurve,
ImportExportTimeSeriesCost, MarketBidTimeSeriesCost, and
ReserveDemandTimeSeriesCurve. Struct fields pin the contained CostCurve to
that shared U; constructors infer and validate.

Replace UnitSystem enum usage in cost-function code with AbstractUnitSystem
type instances (NaturalUnit(), SystemBaseUnit(), DeviceBaseUnit()). The
system-level UnitSystem enum in base.jl / SystemUnitsSettings is left
untouched.

AnyCostCurve{T} alias is used only at isa checks, where existential form is
structurally required.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
So the IS branch this PSY work depends on is discoverable from the repo
rather than requiring local Pkg.develop.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@luke-kiernan luke-kiernan marked this pull request as ready for review April 27, 2026 20:31
@luke-kiernan

luke-kiernan commented Apr 27, 2026

Copy link
Copy Markdown
Contributor Author

Change get_active_power etc. to be non-unitful. Move unitful ones to a different name. Setters are fine as-is.

Does mean for get-then-set you need:
set_active_power!(gen, get_active_power_unitful(gen, SU))

luke-kiernan and others added 2 commits May 6, 2026 16:06
Bare `get_active_power(g, NU)` now returns `100.0`;
`get_active_power_unitful(g, NU)` returns `100.0 MW`. Same pattern for
hand-written `get_base_power` and `get_base_power_{12,23,13}`.

Drop the redundant `Float64` getter dispatch path. Add NU dispatch to
`_convert_from_device_base` for `:mva`/`:ohm`/`:siemens`. Add
`IS._strip_units(::Unitful.Quantity)` so IS stays Unitful-free.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PSB on `psy6` calls 1-arg `get_base_power(::ThermalStandard)` (removed
on this branch), so the four new unit-aware testsets build their
fixtures manually via `_sys_with_thermal` instead of `PSB.build_system`.

Migrate `units/serialization.jl` and `test_units.jl` from JSON3 to JSON
to match the dropped dependency.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@luke-kiernan

luke-kiernan commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

I plan to address #1128 (and am looking for some input on that issue), but otherwise this PR is pretty much finished.

The one other detail that comes to mind: once I go make a compatible branch of PSB, I could also reset those 3 unit tests to using PSB systems. (I currently have them using tiny systems we build here so that the tests pass.)

luke-kiernan and others added 6 commits May 21, 2026 17:53
`set_base_power!` (and `set_base_power_{12,23,13}!`) now accept Float64,
Unitful.Quantity (power), and RelativeQuantity{_,SystemBaseUnit}; passing
a DeviceBaseUnit value errors since device base is 1.0 pu of itself.

Marks the 35 base_power fields as `exclude_setter` in the descriptor,
regenerates structs, hand-writes the setters in components.jl, and drops
redundant overrides on HybridSystem/DynamicGenerator/DynamicInverter
that would otherwise produce dispatch ambiguities. Adds tests covering
all paths.

Also updates the InfrastructureSystems [sources] URL from NREL-Sienna
to Sienna-Platform to match the GitHub org rename.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fa/sa branches multiplied by `MW` while sl and the empty-case zeros are
bare `Float64`. Drop the `* MW` to match.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Covers the empty-system path and a mixed PowerLoad + FixedAdmittance
system. The admittance branches were previously untested, which is how
the DimensionError fixed in 304c1f5 reached main.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@luke-kiernan

luke-kiernan commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

As I'm working my way through all the downstream packages, I'm finding a few more things here and there. I'll post the linked PRs here:

  • PSB: #194 Basically done. Tests pass locally, but there's a few errors logged. Looks like they're pre-existing. Depends on #17 in PowerFlowFileParsers.jl
  • PNM: #308. Done. All tests pass locally. edit: oh, there's merge conflicts.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jd-lara

jd-lara commented May 28, 2026

Copy link
Copy Markdown
Member

Code review

Found 2 issues:

  1. Four flow-limit helper accessors still call the single-argument getter forms that this PR removed, so they will throw MethodError at runtime. get_flow_limits and get_active_power_flow_limits now only exist in their two-argument (component, units) form (see AreaInterchange.jl#L91 and TransmissionInterface.jl#L68), but these callers were not migrated:

"""
function get_from_to_flow_limit(a::AreaInterchange)
return get_flow_limits(a).from_to
end
"""
Get the flow limits from destination [`Area`](@ref) to source [`Area`](@ref) for an [`AreaInterchange`](@ref).
"""
function get_to_from_flow_limit(a::AreaInterchange)
return get_flow_limits(a).to_from
end
"""
Get the minimum active power flow limit for a [`TransmissionInterface`](@ref).
"""
function get_min_active_power_flow_limit(tx::TransmissionInterface)
return get_active_power_flow_limits(tx).min
end
"""
Get the maximum active power flow limit for a [`TransmissionInterface`](@ref).
"""
function get_max_active_power_flow_limit(tx::TransmissionInterface)
return get_active_power_flow_limits(tx).max
end

  1. The with_units_base docstring examples call the removed single-argument get_active_power(gen) form, so the documented usage would now error. Additionally, the new getters take units explicitly and no longer consult the unit state that with_units_base sets, so the docstring's behavior is stale.

PowerSystems.jl/src/base.jl

Lines 543 to 548 in 93c99a5

```julia
active_power_mw = with_units_base(sys, UnitSystem.NATURAL_UNITS) do
get_active_power(gen)
end
# now active_power_mw is in natural units no matter what units base the system is in
```

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@jd-lara jd-lara merged commit 8003bcd into psy6 May 29, 2026
7 of 9 checks passed
@jd-lara jd-lara deleted the lk/units-fold-psu branch May 29, 2026 00:01
jd-lara added a commit that referenced this pull request May 29, 2026
Resolve InterconnectingConverter generated-struct conflict (keep
reactive_power_limits). Pin InfrastructureSystems [sources] to IS4.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
acostarelli pushed a commit that referenced this pull request Jun 9, 2026
Add HybridSystem to the descriptor-driven struct generator
(power_system_structs.json) and regenerate it into
src/models/generated/HybridSystem.jl, replacing the hand-written
src/models/HybridSystem.jl.

This brings HybridSystem in line with the post-#1659 Unitful accessor
convention: unit-bearing getters now strip units and gain _unitful
companions + display_units_arg traits, and setters route through
set_value. It also fixes two latent bugs that the generated code no
longer reproduces: set_interconnection_efficiency! wrote the wrong
field, and three subcomponent setters referenced an undefined `value`.

The non-generatable custom methods move to the supplemental files:
guarded subcomponent setters + _raise_if_attached_to_system into
supplemental_setters.jl; get_subcomponents, _get_components, and
set_units_setting! into supplemental_accessors.jl. The (::Nothing)
constructor still yields concrete subcomponent nulls while the keyword
constructor defaults them to nothing, via the null_value/default split.

Tests: test_hybrid_system, test_subsystems, test_serialization pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants