Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Give MBC test fixture's thermal unit a base power != system base#1650

Open
luke-kiernan wants to merge 1 commit into
mainSienna-Platform/PowerSimulations.jl:mainfrom
lk/mbc-fixture-base-power-mismatchSienna-Platform/PowerSimulations.jl:lk/mbc-fixture-base-power-mismatchCopy head branch name to clipboard
Open

Give MBC test fixture's thermal unit a base power != system base#1650
luke-kiernan wants to merge 1 commit into
mainSienna-Platform/PowerSimulations.jl:mainfrom
lk/mbc-fixture-base-power-mismatchSienna-Platform/PowerSimulations.jl:lk/mbc-fixture-base-power-mismatchCopy head branch name to clipboard

Conversation

@luke-kiernan

@luke-kiernan luke-kiernan commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • load_sys_incr()'s "Test Unit1" had device base_power == get_base_power(sys) (both 100 MVA). Any per-unit multiplier bug that only shows up when a component's rating differs from system base power was invisible to every test built on this shared fixture, including the thermal/hydro MarketBidCost agreement test in both this repo and HydroPowerSimulations.jl.
  • Concretely, this masked Sienna-Platform/HydroPowerSimulations.jl#136: HydroGen's offer-curve breakpoint multiplier used get_max_active_power(d) instead of the 1.0 every other device type uses, and the thermal/hydro-agree test couldn't catch it because the two bases coincided.
  • Sets Test Unit1's base power to 1.4x system base, preserving its natural-units (MW) rating/limits/dispatch via with_units_base(sys, NATURAL_UNITS).

Test plan

  • Rebuilt load_sys_incr() standalone and confirmed unit1.base_power (140.0) now differs from get_base_power(sys) (100.0)
  • test/test_market_bid_cost.jl (full suite, not run here)

Test Unit1's device base_power previously equaled the system's base
power (both 100 MVA), so per-unit multiplier bugs sensitive to that
distinction (e.g. HydroPowerSimulations #136) went uncaught by tests
built on this shared fixture, including the thermal/hydro MarketBidCost
agreement test.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@luke-kiernan

luke-kiernan commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

This tweaks a test so it covers the bug from HPS #136. So until that gets merged, the MBC "compare hydro against thermal" test case should fail. Wouldn't be shocked if a couple other MBC tests fail too--there's some hand-tweaked numbers in those.

@github-actions

Copy link
Copy Markdown
Contributor

Performance Results

Version Precompile Time
Main 3.953650184
This Branch 3.981932072
Version Build Time
Main-Build Time Precompile 134.929016061
Main-Build Time Postcompile 1.677551328
This Branch-Build Time Precompile 127.141623398
This Branch-Build Time Postcompile 1.32925065
Version Solve Time
Main-Solve Time Precompile 547.035650202
Main-Solve Time Postcompile 500.919553495
This Branch-Solve Time Precompile 458.861521443
This Branch-Solve Time Postcompile 409.64680511

@luke-kiernan

luke-kiernan commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

This tweaks a test so it covers the bug from HPS Sienna-Platform/HydroPowerSimulations.jl#136. So until that gets merged, the MBC "compare hydro against thermal" test case should fail. Wouldn't be shocked if a couple other MBC tests fail too--there's some hand-tweaked numbers in those.

Slight misunderstanding on my part: the test that failed lives in HPS and isn't the "compare hydro against thermal" one. But HPS's tests use PSI's helpers in test/test_utils/mbc_system_utils.jl, so the change lands here, not in HPS.

Anyway, the details of the cross-package test failures check out and the change defends against regression as intended.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adjusts the shared load_sys_incr() MarketBidCost test fixture so “Test Unit1” uses a device base power different from the system base power, ensuring per-unit conversion paths are exercised and unit-conversion bugs aren’t masked in downstream tests (including consumers like HydroPowerSimulations).

Changes:

  • Set “Test Unit1” base_power to 1.4x the system base power.
  • Preserve the unit’s natural-units (MW) rating/limits/dispatch by capturing and restoring values inside a with_units_base(..., NATURAL_UNITS) block.

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

Comment on lines +295 to +296
# Give Test Unit1 a device base_power != system base_power, so unit-conversion bugs
# (e.g. rating-dependent per-unit multipliers) aren't masked by the two coinciding.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.