Repository navigation
Fix EnergyReservoirStorage units and ps/qs sign - #76
luke-kiernan wants to merge 3 commits into
Conversation
calculate_gen_rating already returns device-base pu; the extra `rating * base_conversion` (inherited from PSCB) scaled the rating by sys_mbase / mbase whenever MBASE != SBASE. The "larger than base power" clamp compared that device-pu rating against mbase in MVA; it now compares against 1.0 pu. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
_make_per_unit! leaves storage energy/ratings/qmin/qmax in system-base pu, but make_storage! (inherited from PSCB) multiplied them by the raw, system-pu thermal_rating and used that as base_power too. On case5_strg.m a 100 MW / 100 MWh battery came out as 1 MVA / 1 MWh. Convert with sys_mbase, which is also the base_power now. _make_per_unit!/_make_mixed_units! never rescaled storage ps/qs, though PowerModels documents them in MW/MVAr and treats them as pu internally (e.g. the bus power-mismatch calculation). Rescale them with the other storage fields. PowerModels defines ps/qs as power withdrawn, while PSY's active_power/reactive_power are injections, so negate them. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
While this is a behavioral change, it's pretty unambiguous that this is an improvement. Right now, the values for the storage component's fields are in the 0.1-1 MW range; with this change, they're in the 10-100 MW range, which seems much more plausible. |
There was a problem hiding this comment.
🟡 Changes recommended
Storage reactive bounds retain the wrong sign convention, and renewable rating clamping truncates valid capacity.
2 open findings
What changed in this PR
Corrects MATPOWER storage units and power signs in OpenAPI output, continuing the system-base conversion corrections from #75.
Changes:
- Rescales storage
ps/qsand converts storage quantities using the system base. - Removes duplicate renewable rating conversion and changes rating clamping.
- Updates storage and renewable regression tests.
| File | Description |
|---|---|
| test/test_parse_matpower.jl | Adds storage ps/qs conversion round-trip coverage. |
| test/test_openapi_generation.jl | Updates storage values and renewable rating expectations. |
| src/pm_io/data.jl | Rescales storage ps/qs between unit systems. |
| src/openapi/generation.jl | Changes storage scaling, signs, and renewable ratings. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if rating > 1.0 | ||
| @warn "rating is larger than base power for $gen_name, setting to $mbase" | ||
| rating = mbase | ||
| rating = 1.0 |
There was a problem hiding this comment.
I believe this is intentional and a pre-existing design thing
qmin/qmax bound qs, which is withdrawal; the injection-sign reactive_power needs bounds [-qmax, -qmin]. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Stacked on #75; review that first. This PR's diff is the one commit on top of it.
Problem
make_storage!multiplied every system-per-unit storage field by the rawthermal_rating, which is itself still system per-unit, and used that same number asbase_power. This was ported deliberately from PSCB'smake_generic_batteryfor parity. Oncase5_strg.m, the only storage case in PowerSystemsTestData (PSB'smatpower_case5_strg_sys), every storage power and energy value came out 100x too small: the file's 100 MW / 100 MWh batteries came out as 1 MVA / 1 MWh.ps/qshad two more problems:ps/qsin MW/MVAr inmpc.storageand treats them as per-unit internally; for example, the power-mismatch calculation addspstopd/pg. But_make_per_unit!never rescaled them, here or upstream. That gap never showed becauseps = qs = 0in every case on hand.ps/qsas power withdrawn (positive when charging). PSY'sEnergyReservoirStorage.active_power/reactive_powerare injections: PowerFlows.jl adds them to the bus injections, uses(0, output_active_power_limits.max)as the power-flow limits, and exports storage as a generator withPG = active_power.Fix
_make_per_unit!/_make_mixed_units!now rescale storageps/qsalong with the other storage fields.make_storage!converts every storage power and energy field withsys_mbase, setsbase_power = sys_mbase, and negatesps/qs.case5_strg.mtest now checks the natural values from the file. Before, it only comparedratingwithbase_power, which hid the 100x error. The synthetic storage tests are rewritten to match, and there's a new round-trip test for theps/qsper-unit conversion.Compatibility
This changes output for every MATPOWER case with storage: on
case5_strg, storage values grow 100x. There's no API change.PowerModelsDatadicts now carryps/qsin per-unit, like the other storage fields. The PSS/E path is unaffected, because it produces no storage.Open question
base_power: should it besys_mbase(this PR) or the battery's own naturalthermal_rating(100 MVA oncase5_strg)? PowerModels storage has no device base of its own. The natural-unit output is the same either way; onlyCOMPONENT_BASEdocuments differ.thermal_ratingcan be 0, so using it would need a fallback.🤖 Generated with Claude Code