Lk/units prototype rebase by luke-kiernan · Pull Request #570 · Sienna-Platform/InfrastructureSystems.jl

luke-kiernan · 2026-04-15T22:41:26Z

Add unitful setters and getters. The new dependency (which I named PowerSystemsUnits.jl, open to other suggestions) could be folded into IS, but I'd prefer to keep it separate.

- Add units.jl with Unitful integration, RelativeQuantity type for DU/SU - Define custom Mvar and MVA units for reactive/apparent power - Update generate_structs.jl template to generate two-method accessors: - Default accessor returns natural units (MW, Mvar, etc.) - Optional second argument accepts explicit unit (DU, SU, MW, etc.) - Add get_natural_unit() to map conversion types to appropriate units (reactive power fields → Mvar, other power fields → MW) - Export unit types and Unitful re-exports from main module - Register custom Unitful units in __init__() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

The type parameter in repr() may or may not be module-qualified depending on what names are in scope, which differs between local and CI environments. Use occursin checks instead of exact string matching. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

When exclude_getter is true, generates an internal _get_ prefixed accessor instead of the public get_ accessor. Suppresses the public getter export while keeping setter exports (since exclude_setter means hand-written, not nonexistent). Used by PSY to make get_base_power return unitful values while keeping _get_base_power as raw Float64 for internal conversion machinery. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

The 1-arg getter now uses DEFAULT_UNITS (a constant) instead of the stateful _get_system_units, eliminating type instability and enabling full compiler optimization of the getter path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Replace inline units.jl with using/re-exporting from PowerSystemsUnits. Keep time_period_conversion in units.jl (unrelated to unit types). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

jd-lara

Move the code to be a Module in IS or split the functionality between IS and PSY accordingly.

Copilot

Pull request overview

This PR rebases the “units prototype” work by introducing unit-aware accessor generation and integrating the new PowerSystemsUnits.jl dependency into InfrastructureSystems’ public surface, alongside updates to curve printing and related tests.

Changes:

Re-export unit types/utilities from PowerSystemsUnits via InfrastructureSystems and add new package dependencies (PowerSystemsUnits, Unitful).
Adjust ValueCurve/cost-alias printing so alias names (e.g., LinearCurve, QuadraticCurve) are used more consistently.
Relax repr-string tests for cost curves to be less sensitive to module-qualification differences.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`test/test_cost_functions.jl`	Makes `repr` tests less brittle by switching to substring assertions.
`src/value_curve.jl`	Simplifies `simple_type_name` to rely on `nameof(typeof(...))`, with alias overrides handled elsewhere.
`src/utils/generate_structs.jl`	Extends the struct generator template to support unitful getters/setters and introduces exclude-getter logic.
`src/cost_aliases.jl`	Adds alias-specific `simple_type_name` overrides and updates `show` output to use alias names.
`src/InfrastructureSystems.jl`	Re-exports units/types/helpers from `PowerSystemsUnits`.
`Project.toml`	Adds `PowerSystemsUnits` and `Unitful` dependencies + compat bounds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-15T22:50:04Z

+# Map conversion_unit to the default natural Unitful unit for getters
+const NATURAL_UNIT_MAP = Dict{String, String}(
+    ":mva" => "MW",           # Power → MW (default for :mva)
+    ":ohm" => "OHMS",         # Impedance → Ohms
+    ":siemens" => "SIEMENS",  # Admittance → Siemens
+)
+
+# Determine the natural unit based on conversion_unit and field name
+function get_natural_unit(conversion_unit::String, field_name::String)
+    if conversion_unit == ":mva"
+        # Reactive power fields use Mvar instead of MW
+        if occursin("reactive", lowercase(field_name))
+            return "Mvar"
+        else
+            return "MW"
+        end
+    end
+    return get(NATURAL_UNIT_MAP, conversion_unit, "MW")
+end
+


NATURAL_UNIT_MAP, get_natural_unit, and the computed natural_unit field are currently unused in the Mustache STRUCT_TEMPLATE (no {{natural_unit}} reference). This adds dead code/complexity to the generator. Either wire natural_unit into the generated getter/docstring behavior (and add coverage) or remove these definitions until they’re needed.

Suggested change

# Map conversion_unit to the default natural Unitful unit for getters

const NATURAL_UNIT_MAP = Dict{String, String}(

":mva" => "MW", # Power → MW (default for :mva)

":ohm" => "OHMS", # Impedance → Ohms

":siemens" => "SIEMENS", # Admittance → Siemens

)

# Determine the natural unit based on conversion_unit and field name

function get_natural_unit(conversion_unit::String, field_name::String)

if conversion_unit == ":mva"

# Reactive power fields use Mvar instead of MW

if occursin("reactive", lowercase(field_name))

return "Mvar"

else

return "MW"

end

end

return get(NATURAL_UNIT_MAP, conversion_unit, "MW")

end

Copilot · 2026-04-15T22:50:05Z

+    VoltageCategory, CurrentCategory
+export POWER, IMPEDANCE, ADMITTANCE, VOLTAGE, CURRENT
+export convert_units, base_value, system_base_value, natural_unit, DEFAULT_UNITS
+export get_device_base_power, get_system_base_power, get_base_voltage


This file contains a hard rule comment saying not to add exports due to name clashes, but this change re-exports several functions/constants from PowerSystemsUnits (e.g., convert_units, ustrip, DEFAULT_UNITS). Either update/remove the "Do not add export statements" guidance to reflect the intended public API, or avoid re-exporting functions and require consumers to qualify them via PowerSystemsUnits to prevent collisions.

Suggested change

export get_device_base_power, get_system_base_power, get_base_voltage

Copilot · 2026-04-15T22:50:05Z

+    @test occursin("QuadraticCurve(1.0, 2.0, 3.0)", repr(cc))
+    @test occursin("LinearCurve(0.0, 0.0)", repr(cc))
+    @test repr(fc) == sprint(show, fc)
+    @test occursin("FuelCurve", repr(fc))
+    @test occursin("QuadraticCurve(1.0, 2.0, 3.0)", repr(fc))


The updated repr assertions for CostCurve/FuelCurve no longer check that the unit system (e.g., NATURAL_UNITS) is present in the representation. This makes it easier for a regression to drop or misreport units without failing tests. Consider adding an occursin assertion for the unit-system portion of the string (while still avoiding module-qualification brittleness).

Suggested change

@test occursin("QuadraticCurve(1.0, 2.0, 3.0)", repr(cc))

@test occursin("LinearCurve(0.0, 0.0)", repr(cc))

@test repr(fc) == sprint(show, fc)

@test occursin("FuelCurve", repr(fc))

@test occursin("QuadraticCurve(1.0, 2.0, 3.0)", repr(fc))

@test occursin("QuadraticCurve(1.0, 2.0, 3.0)", repr(cc))

@test occursin("NATURAL_UNITS", repr(cc))

@test occursin("LinearCurve(0.0, 0.0)", repr(cc))

@test repr(fc) == sprint(show, fc)

@test occursin("FuelCurve", repr(fc))

@test occursin("QuadraticCurve(1.0, 2.0, 3.0)", repr(fc))

@test occursin("NATURAL_UNITS", repr(fc))

luke-kiernan · 2026-05-05T18:16:42Z

Superseded by #577

luke-kiernan and others added 8 commits April 15, 2026 16:21

adjust struct generation

5a88a34

tests now pass

b02e7d5

Use PowerSystemsUnits.jl for unit types and conversions

1e89b78

Replace inline units.jl with using/re-exporting from PowerSystemsUnits. Keep time_period_conversion in units.jl (unrelated to unit types). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

put back default is_cost_alias (lost in rebase)

0a3b486

luke-kiernan requested review from Copilot and jd-lara April 15, 2026 22:42

Copilot started reviewing on behalf of luke-kiernan April 15, 2026 22:42 View session

jd-lara reviewed Apr 15, 2026

View reviewed changes

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lk/units prototype rebase#570

Lk/units prototype rebase#570
luke-kiernan wants to merge 8 commits intoIS4from
lk/units-prototype-rebase

luke-kiernan commented Apr 15, 2026 •

edited

Loading

Uh oh!

jd-lara left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 15, 2026

Uh oh!

Copilot AI Apr 15, 2026

Uh oh!

Copilot AI Apr 15, 2026

Uh oh!

luke-kiernan commented May 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

luke-kiernan commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jd-lara left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

luke-kiernan commented May 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

luke-kiernan commented Apr 15, 2026 •

edited

Loading