Lk/units domain agnostic is4#577
Open
luke-kiernan wants to merge 16 commits intoIS4from
Open
Conversation
- 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>
- Purge power-domain knowledge from generate_structs template (NATURAL_UNIT_MAP / get_natural_unit removed); template emits untyped `units` parameter for downstream packages to specialize. - Declare bare `get_value` / `set_value` interface; methods are now provided by domain packages (e.g. PowerSystems). - Drop PowerSystemsUnits and Unitful deps so IS has no knowledge of power-system-specific unit types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- New relative_units.jl vendors AbstractRelativeUnit, DU/SU/NU, and
RelativeQuantity; power-specific types stay in PSY.
- Template drops the 1-arg getter emission — callers must specify units.
Emits a `display_units_arg(f, ::Type{T})` trait so consumers can dispatch
on whether a (getter, struct) pair takes units.
- `_make_time_array` now calls `multiplier(owner, SU)` so
`scaling_factor_multiplier = get_max_active_power` keeps working.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror the existing exclude_setter behavior so the public name is registered for export even when the generator emits a _-prefixed internal accessor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## IS4 #577 +/- ##
==========================================
- Coverage 82.04% 81.75% -0.29%
==========================================
Files 74 75 +1
Lines 6238 6391 +153
==========================================
+ Hits 5118 5225 +107
- Misses 1120 1166 +46
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Add simple_type_name overrides for the five TimeSeries*Curve aliases and
use them in Base.show instead of typeof(vc), so output is e.g.
TimeSeriesLinearCurve(...) rather than the full parametric
TimeSeriesInputOutputCurve{TimeSeriesFunctionData{LinearFunctionData}}(...).
Brings these aliases to parity with the non-TS aliases.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add U <: AbstractUnitSystem as a second type parameter on ProductionVariableCostCurve, replacing the runtime power_units field. get_power_units returns U(). Eliminates Val-wrapping in downstream unit-dependent dispatch. Introduces AbstractUnitSystem as a common supertype of AbstractRelativeUnit and NaturalUnit. Constructors default U = NaturalUnit; serialize/deserialize encode U under "power_units". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- AnyCostCurve{T}, AnyFuelCurve{T}, AnyProductionVariableCostCurve{T}:
existential UnionAll aliases for downstream code at isa sites or
method signatures where the U parameter is irrelevant.
- _unit_system_instance now accepts both new type-name strings
("NaturalUnit") and legacy enum-value strings ("NATURAL_UNITS") so
existing serialized fixtures keep deserializing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cost coefficients convert as the inverse of the power-value ratio (squared for quadratic terms). Bases pass in as Float64 args so IS needs no component interface stubs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Covers identity, all 6 cross-system conversions, exponent for quadratic, round-trip identity, and the negative-exponent (power-value-ratio) case used by IOM piecewise x-coord rescaling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR continues the “domain-agnostic units” work by introducing relative unit-system markers (DU/SU/NU), updating printing/display paths to support unit-aware getters, and refactoring production-variable cost curves to encode power_units at the type level (with corresponding serialization + test updates).
Changes:
- Add domain-agnostic unit markers (
DeviceBaseUnit,SystemBaseUnit,NaturalUnit) plusRelativeQuantityarithmetic/helpers. - Refactor
CostCurve/FuelCurve(and their supertype) to carrypower_unitsas a type parameter; update show/serialization accordingly. - Update PrettyTables component display and time-series scaling-factor application to optionally pass a units marker to getters/multipliers; adjust tests for new unit markers and printing.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_time_series_function_data.jl | Updates CostCurve construction/assertions to use new unit-marker instances. |
| test/test_relative_units.jl | Adds coverage for RelativeQuantity construction, arithmetic, comparisons, display, and cost coefficient conversion. |
| test/test_make_convex.jl | Updates unit expectations for convexification results to new unit markers. |
| test/test_cost_functions.jl | Makes printing tests resilient to qualification changes and updates expected unit-marker formatting/order. |
| src/value_curve.jl | Simplifies simple_type_name and relies on alias-specific overrides for display names. |
| src/utils/test.jl | Adds 2-arg getter overloads so test scaling multipliers can accept a units marker. |
| src/utils/print_pt_v3.jl | Passes an optional units argument to getters based on display_units_arg. |
| src/utils/print_pt_v2.jl | Same as v3, for PrettyTables v2 compatibility. |
| src/utils/generate_structs.jl | Updates generator template to emit unit-aware 2-arg getters when conversion is needed and adds display_units_arg trait methods. |
| src/time_series_interface.jl | Changes scaling-factor application to call multipliers with a units marker (SU). |
| src/relative_units.jl | Introduces unit marker types, RelativeQuantity, convert_cost_coefficient, and the display_units_arg trait default. |
| src/production_variable_cost_curve.jl | Refactors cost curves to carry power_units as a type parameter; adds (de)serialization logic and updated show methods. |
| src/cost_aliases.jl | Restores/relocates is_cost_alias default and adds simple_type_name overrides for alias printing consistency. |
| src/InfrastructureSystems.jl | Declares/exports the unit conversion interface hooks (get_value, set_value) and includes relative_units.jl. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+953
to
+956
| # Scaling-factor multipliers (e.g. `get_max_active_power`) are unit-aware | ||
| # accessors from downstream packages; pass `SU` so the result is in the | ||
| # system base that consumers of the time series expect. | ||
| return ta .* multiplier(owner, SU) |
There was a problem hiding this comment.
_make_time_array now always calls multiplier(owner, SU), but the public API/docs for scaling_factor_multiplier (and existing callers) treat it as a 1-arg function multiplier(owner). This will throw a MethodError for any multiplier that hasn’t been updated to accept a units marker. Consider keeping backward compatibility by calling the 2-arg form only when applicable (or try/catch MethodError and fall back to 1-arg), and update the docstrings accordingly so the required signature is unambiguous.
Contributor
Author
There was a problem hiding this comment.
For plotting or printing, people like to see natural units. Add additional argument?
| elseif name in ("DEVICE_BASE", "DeviceBaseUnit") | ||
| return DeviceBaseUnit() | ||
| end | ||
| T = getproperty(@__MODULE__, Symbol(name)) |
There was a problem hiding this comment.
_unit_system_instance uses getproperty(@__MODULE__, Symbol(name)) for non-legacy strings. If name is unknown, this will throw a low-level UndefVarError/type has no field rather than a clear ArgumentError explaining valid unit markers. Consider checking isdefined(@__MODULE__, Symbol(name)) (or wrapping in try/catch) and throwing an ArgumentError with the supported values to improve debuggability and error reporting for corrupted/hand-edited serialized data.
Suggested change
| T = getproperty(@__MODULE__, Symbol(name)) | |
| sym = Symbol(name) | |
| if !isdefined(@__MODULE__, sym) | |
| throw( | |
| ArgumentError( | |
| "Unsupported power_units value \"$name\". Supported values include " * | |
| "\"NATURAL_UNITS\", \"SYSTEM_BASE\", \"DEVICE_BASE\", " * | |
| "\"NaturalUnit\", \"SystemBaseUnit\", and \"DeviceBaseUnit\".", | |
| ), | |
| ) | |
| end | |
| T = getproperty(@__MODULE__, sym) |
Comment on lines
+22
to
+23
| @test_throws Exception 0.6 * IS.DU + 0.4 * IS.SU | ||
| @test_throws Exception 0.6 * IS.DU == 0.4 * IS.SU |
There was a problem hiding this comment.
These assertions expect a very broad Exception. Since DU/SU mixing currently fails due to missing methods, it should deterministically raise a MethodError. Tightening @test_throws to MethodError makes the test more precise and avoids accidentally passing on unrelated failures.
Suggested change
| @test_throws Exception 0.6 * IS.DU + 0.4 * IS.SU | |
| @test_throws Exception 0.6 * IS.DU == 0.4 * IS.SU | |
| @test_throws MethodError 0.6 * IS.DU + 0.4 * IS.SU | |
| @test_throws MethodError 0.6 * IS.DU == 0.4 * IS.SU |
luke-kiernan
commented
Apr 27, 2026
| @@ -0,0 +1,165 @@ | |||
| ############################### | |||
Contributor
Author
There was a problem hiding this comment.
Move contents of this file to a submodule.
I'm also slightly bothered by the fact that you can do 1 SU (relative unit) but not 1 NU (dispatch flag only).
Comment on lines
+953
to
+956
| # Scaling-factor multipliers (e.g. `get_max_active_power`) are unit-aware | ||
| # accessors from downstream packages; pass `SU` so the result is in the | ||
| # system base that consumers of the time series expect. | ||
| return ta .* multiplier(owner, SU) |
Contributor
Author
There was a problem hiding this comment.
For plotting or printing, people like to see natural units. Add additional argument?
| `power_units` at construction is the singleton instance `U()` (default `NaturalUnit()`). | ||
| """ | ||
| @kwdef struct FuelCurve{T <: ValueCurve} <: ProductionVariableCostCurve{T} | ||
| struct FuelCurve{T <: ValueCurve, U <: AbstractUnitSystem} <: |
Contributor
Author
There was a problem hiding this comment.
Need units on the y axis too: might be gallons, MMBTU, Joules...
Contributor
Author
|
Also delete print version 2. pending on openAPI/schema integration. |
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.
Continuation of #574. I need to add the
power_unitsfield as a type parameter on theCostCurve, but then this will be ready to merge.