Repository navigation
Convert 2W transformer ratings/flows with the system base - #75
Open
luke-kiernan wants to merge 2 commits into
Open
luke-kiernan wants to merge 2 commits into
luke-kiernan wants to merge 2 commits into
Conversation
rate_a/rate_b/rate_c/pf/qf are system-base pu after _make_per_unit!, so multiplying by the winding base (SBASE1-2) scaled them by SBASE1-2/SBASE. Use sys_mbase, as make_line! does. Output only changes when SBASE1-2 != SBASE; the synthetic discriminator test, which had pinned the old values, now expects the raw RATE11 MVA. Fixes #74 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! |
PTRAN is "MW comprising this transfer" per the PSS/E manual, and _make_per_unit! never touches interarea_transfer, so power_transfer is already natural MW. Multiplying by sys_mbase (inherited from PSCB) inflated it 100x on a 100 MVA system. Adds a v35 fixture with a real inter-area transfer record, since none existed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
luke-kiernan
marked this pull request as ready for review
October 15, 2026 14:50
luke-kiernan
requested review from
hannahchubin and
jd-lara
and
a balanced review from Copilot
October 15, 2026 16:32
There was a problem hiding this comment.
🟢 Approval recommended
The conversions match the parser’s unit conventions, with only an optional regression-coverage improvement remaining.
1 open finding
What changed in this PR
Corrects unit conversions in the OpenAPI reader to address #74 while preserving device-base impedances.
Changes:
- Converts two-winding transformer ratings and flows using the system base.
- Preserves inter-area transfers in natural MW.
- Updates regression expectations and adds an inter-area transfer fixture.
| File | Description |
|---|---|
| test/test_openapi_transformer_discriminators.jl | Corrects unequal-base rating expectations. |
| test/test_openapi_dc_shunt.jl | Tests natural-MW inter-area transfers. |
| test/test_openapi_branch.jl | Updates transformer assertions and explanatory comments. |
| test/fixtures/synthetic_v35_interarea_transfer.raw | Adds a 25 MW transfer case. |
| src/openapi/dc_branch.jl | Removes redundant transfer scaling. |
| src/openapi/branch.jl | Uses the system base for transformer ratings and flows. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+433
to
+434
| rating_b = _natural_value(rate_b, sys_mbase), | ||
| rating_c = _natural_value(rate_c, sys_mbase), |
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.

Fixes #74, plus a related
AreaInterchangefix in a separate commit._make_per_unit!leavesrate_a/rate_b/rate_c/pf/qfin system-base pu, butmake_transformer_2w!converted them to natural units with the circuit's winding base (SBASE1-2). That scaled ratings and flows bySBASE1-2 / SBASE. Now they're converted withsys_mbase, asmake_line!already does. Impedances and the magnetizing shunt are unchanged, since they really are device-base.Compatibility
make_transformer_2w!is internal and gains asys_mbaseargument, matchingmake_line!.SBASE1-2 ≠ SBASE(andSBASE1-2 ≠ 0). MATPOWER input andmodified_14bus_system.raware unaffected.synthetic_v35_transformer_discriminators.raw, had tests pinning the old behavior and labeling it "right". They now expect the rawRATE11MVA: T1 is 40 instead of 20, and T2 is 64 instead of 51.2. Those two transformers already act as the regression case from the issue, so I didn't add a new fixture.Second commit:
AreaInterchangeflowThe same kind of inherited scaling bug. The PSS/E manual defines
PTRANas "MW comprising this transfer", and_make_per_unit!never touchesinterarea_transfer, sopower_transferis already natural MW.read_area_interchanges!multiplied it bysys_mbaseanyway: on a 100 MVA system, 25 MW came out as 2500 MW. The new fixturesynthetic_v35_interarea_transfer.rawis the first to contain an actual inter-area transfer record, and its test replaces the one that pinned the inflated value. Output changes for any PSS/E case with a nonzeroPTRAN.🤖 Generated with Claude Code