Use IS.convert_cost_coefficient for unit-system normalization by luke-kiernan · Pull Request #91 · Sienna-Platform/InfrastructureOptimizationModels.jl

luke-kiernan · 2026-04-27T20:28:49Z

Replaces the Val{IS.UnitSystem.X}() cost-coefficient dispatch with the new IS.convert_cost_coefficient primitive, eliminating the value-to-type boundary inside the cost-objective hot path. Net -137 lines in src/utils/component_utils.jl; 12 method bodies collapse to 4 one-liners.

Stacks on:

IS lk/units-domain-agnostic-is4 — adds IS.AbstractUnitSystem, parameterizes CostCurve/FuelCurve on U, adds IS.convert_cost_coefficient.
PSY lk/units-fold-psu — sweeps PSY for the new types.

Pinned via [sources] in Project.toml and test/Project.toml.

Summary

bee133c — pin IS to lk/units-domain-agnostic-is4.
d1e831b — rewrite the 4 cost-coefficient helpers in component_utils.jl to delegate to IS.convert_cost_coefficient. Public function names and call-site behavior preserved; only internals change. Signatures move from IS.UnitSystem to IS.AbstractUnitSystem. Test files swept for IS.UnitSystem.X → IS.X().
b08af0e — add scripts/units_dispatch_profile.jl for empirically validating type-stability claims (see Type stability of cost-coefficient conversion: where it is, where it isn't #90). JET + BenchmarkTools are local-only deps; install per the script header.

Type stability

Issue #90 documents the full picture. Short version:

Val{}-on-runtime-enum (the old antipattern) is gone.
Per-call-site method specialization carries concrete U through the conversion math automatically.
@code_warntype confirms power_units::Core.Const(NU) inside the specialization — conversion folds at compile time.
Single remaining boundary: one method-table lookup per add_variable_cost_to_objective! call. Cheap to address later via Union field typing if profiling flags it; not blocking.

Test plan

All 1202 existing IOM tests pass — they exercise the helpers' input/output for all three unit systems × linear/quadratic/piecewise paths and serve as the regression guard for the refactor.
IS-side: 22 new tests for convert_cost_coefficient (identity, all 6 cross-system pairs, exponent for quadratic, round-trip identity, negative-exponent piecewise case).
Profile script (scripts/units_dispatch_profile.jl) confirms: 1 dynamic dispatch site (entry point, expected), full type stability past entry, ~968μs / 31k allocs for N=50 components × 24 timesteps.

🤖 Generated with Claude Code

github-actions · 2026-04-27T20:36:43Z

Performance Results
Main

This branch

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

The 4 per-system-unit helpers in component_utils.jl each dispatched on Val{IS.UnitSystem.X} through 3 method bodies. With the new AbstractUnitSystem types and IS.convert_cost_coefficient (cost coefficient = inverse of power-value ratio, exponent for quadratic), each helper collapses to a single call. Net -137 lines. Signatures move from IS.UnitSystem to IS.AbstractUnitSystem; tests updated to pass IS.NaturalUnit() etc. instead of enum values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Uses test mocks to run JET.@report_opt + @Btime + @code_warntype on the cost-objective construction path. Empirically backs the rung-2 claim in issue #90. JET and BenchmarkTools intentionally not in test/Project.toml; install them locally per the script header before running. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Refactors unit-system normalization for cost coefficients to use the new InfrastructureSystems.convert_cost_coefficient API, removing the previous Val{IS.UnitSystem.*} dispatch pattern from IOM’s cost-objective path and updating call sites/tests to the new unit-system singletons.

Changes:

Replaced cost-coefficient helper internals with thin wrappers over IS.convert_cost_coefficient and updated signatures to IS.AbstractUnitSystem.
Swept tests to use IS.NaturalUnit() / IS.SystemBaseUnit() / IS.DeviceBaseUnit() instead of IS.UnitSystem.*.
Added a profiling script (scripts/units_dispatch_profile.jl) to empirically inspect remaining dispatch and wall-time costs.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
`src/utils/component_utils.jl`	Collapses cost normalization helpers onto `IS.convert_cost_coefficient`; updates piecewise normalization implementation.
`src/objective_function/cost_term_helpers.jl`	Updates helper signature to accept `IS.AbstractUnitSystem`.
`src/objective_function/value_curve_cost.jl`	Updates internal function signatures to accept `IS.AbstractUnitSystem`.
`src/InfrastructureOptimizationModels.jl`	Removes the `set_units_base_system!` extension-point stub.
`src/core/optimization_container.jl`	Removes prior unit-system hook and leaves a commented migration note in container init.
`scripts/units_dispatch_profile.jl`	Adds a local-only profiling script using test mocks + JET/BenchmarkTools.
`Project.toml`	Pins `InfrastructureSystems` to the units branch via `[sources]`.
`test/Project.toml`	Mirrors the `InfrastructureSystems` source pin for the test environment.
`test/test_ts_value_curve_objective.jl`	Updates tests to new unit-system singleton API.
`test/test_quadratic_curve.jl`	Updates tests to new unit-system singleton API.
`test/test_pwl_methods.jl`	Updates tests to new unit-system singleton API.
`test/test_piecewise_linear.jl`	Updates tests to new unit-system singleton API.
`test/test_linear_curve.jl`	Updates tests to new unit-system singleton API.
`test/test_cost_term_helpers.jl`	Updates tests to new unit-system singleton API.

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

    points = cost_component.points
    points_normalized = Vector{NamedTuple{(:x, :y)}}(undef, length(points))
    for (ix, point) in enumerate(points)
-        points_normalized[ix] = (x = point.x / system_base_power, y = point.y)
+        points_normalized[ix] = (x = point.x * x_ratio, y = point.y)
    end


-    device_base_power::Float64,
-)
    points = cost_component.points
    points_normalized = Vector{NamedTuple{(:x, :y)}}(undef, length(points))


+    y_ratio = IS.convert_cost_coefficient(
+        1.0, unit_system, IS.SU,
+        system_base_power, device_base_power, 1,
+    )


@@ -110,7 +110,6 @@ function get_operation_cost end
 function get_dc_bus end
 function get_bustype end
 function has_service end


-    temp_set_units_base_system!(sys, "SYSTEM_BASE")
    # The order of operations matter
+    # stateful unit system is being phased out; POM should no longer need this.
+    # temp_set_units_base_system!(sys, "SYSTEM_BASE")


    points = cost_component.points
    points_normalized = Vector{NamedTuple{(:x, :y)}}(undef, length(points))
    for (ix, point) in enumerate(points)
-        points_normalized[ix] = (x = point.x / system_base_power, y = point.y)
+        points_normalized[ix] = (x = point.x * x_ratio, y = point.y)
    end


-    device_base_power::Float64,
-)
    points = cost_component.points
    points_normalized = Vector{NamedTuple{(:x, :y)}}(undef, length(points))


+    y_ratio = IS.convert_cost_coefficient(
+        1.0, unit_system, IS.SU,
+        system_base_power, device_base_power, 1,
+    )


@@ -110,7 +110,6 @@ function get_operation_cost end
 function get_dc_bus end
 function get_bustype end
 function has_service end


-    temp_set_units_base_system!(sys, "SYSTEM_BASE")
    # The order of operations matter
+    # stateful unit system is being phased out; POM should no longer need this.
+    # temp_set_units_base_system!(sys, "SYSTEM_BASE")


kennedy-mindermann linked an issue May 21, 2026 that may be closed by this pull request

Type stability of cost-coefficient conversion: where it is, where it isn't #90

Closed

luke-kiernan and others added 5 commits May 27, 2026 13:28

Pin IS to lk/units-domain-agnostic-is4 via [sources]

2b550cf

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

fix one test

40b48db

remove stateful unit system stubs

b85473a

luke-kiernan force-pushed the lk/units-domain-agnostic branch from b08af0e to b85473a Compare May 27, 2026 19:48

jd-lara requested review from Copilot and jd-lara May 28, 2026 23:29

Copilot started reviewing on behalf of jd-lara May 28, 2026 23:29 View session

Copilot AI reviewed May 28, 2026

View reviewed changes

jd-lara added 2 commits May 28, 2026 18:02

Merge remote-tracking branch 'origin/main' into lk/units-domain-agnostic

73fafd5

fixes for testing

bd502fe

jd-lara approved these changes May 29, 2026

View reviewed changes

jd-lara merged commit e33ee44 into main May 29, 2026
5 of 6 checks passed

jd-lara deleted the lk/units-domain-agnostic branch May 29, 2026 00:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use IS.convert_cost_coefficient for unit-system normalization#91

Use IS.convert_cost_coefficient for unit-system normalization#91
jd-lara merged 7 commits into
mainfrom
lk/units-domain-agnostic

luke-kiernan commented Apr 27, 2026

Uh oh!

github-actions Bot commented Apr 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

luke-kiernan commented Apr 27, 2026

Summary

Type stability

Test plan

Uh oh!

github-actions Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Apr 27, 2026 •

edited

Loading