Fix docs build: add check_parallel_branch_type_consistency to public API reference#1693
Fix docs build: add check_parallel_branch_type_consistency to public API reference#1693mcllerena wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves robustness when parsing and validating parallel two-terminal AC branches by introducing tolerance-based normalization (tap ratio, phase shift, and near-zero impedance) and adding a system-level check to warn about mixed-type parallel branches that can break downstream reductions.
Changes:
- Added tap/phase-shift tolerances to demote “degenerate” transformers to simpler equivalent types during parsing.
- Added arc-level normalization rules for certain mixed-type parallel branch groups (e.g., Transformer2W/TapTransformer, Line/DiscreteControlledACBranch).
- Added
check_parallel_branch_type_consistency(sys)and wired it intocheck(sys)to warn on mixed-type parallel branch arcs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/IO/system_checks.jl | Adds a new system check to detect mixed-type parallel branches on the same arc. |
| src/PowerSystems.jl | Exports the new check_parallel_branch_type_consistency API. |
| src/parsers/power_models_data.jl | Adds tolerance-based transformer demotion and parallel-arc branch-type normalization/overrides in the PowerModels data parser. |
| src/definitions.jl | Introduces new parser tolerances/constants used for normalization decisions. |
| src/base.jl | Runs the new parallel-branch consistency check as part of check(sys). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This reverts commit c3bd767.
…ansformers - Move `!d["transformer"]` guard into the initial zero-impedance DiscreteControlledACBranch check in `get_branch_type_psse`; transformer sections with zero impedance must remain transformers (Transformer2W), not become switches - Update "Test conversion zero impedance branch to switch" expectations from (6,4) to (3,1): the old values reflected broken behavior where zero-impedance transformer sections were erroneously classified as DiscreteControlledACBranch - Add targeted regression tests for tolerance-based branch-type normalization (PSS/E and Matpower): near-identity tap demotion to Transformer2W, near-zero shift demotion, controllable transformer exemptions, and zero-impedance non-transformer switch detection
| const PARSER_TAP_RATIO_CORRECTION_TOL = 1e-5 | ||
|
|
||
| const ZERO_IMPEDANCE_REACTANCE_THRESHOLD = 1e-4 | ||
| const ZERO_IMPEDANCE_RESISTANCE_THRESHOLD = 1e-6 |
There was a problem hiding this comment.
How do we set this value? My impression from PSSE is that a line is only considered zero impedance if the resistance is zero and the reactance is below the user defined threshold (with default 1e-4). Just wondering what the logic is for having it be non-zero. Does this change significantly the number of parsed zero-impedance branches in the EI?
| if d["group_number"] == WindingGroupNumber.UNDEFINED || is_alpha_controllable | ||
| # Degenerate PST: unrecognized vector group with near-zero shift → demote | ||
| if is_zero_shift && !is_alpha_controllable | ||
| return is_identity_tap ? Transformer2W : TapTransformer |
There was a problem hiding this comment.
Shouldn't we also be checking for is_tap_controllable as well on this path when deciding between Transformer2W and TapTransformer here? Similar to line 1267?
There was a problem hiding this comment.
This path when the winding group number is undefined I don't think is explicitly tested.
| end | ||
|
|
||
| function _expected_discrete_state(d::Dict, bus_f::ACBus, bus_t::ACBus) | ||
| available = d["br_status"] == 1 |
There was a problem hiding this comment.
use _is_active_pti_branch
| n == 0 || @warn "System has \$n arcs with mixed-type parallel branches" | ||
| ``` | ||
| """ | ||
| function check_parallel_branch_type_consistency(sys::System) |
There was a problem hiding this comment.
I wonder if this function should distinguish between cases where both branches are available (will actually cause an issue in the network reduction/modeling) and cases where you have parallel mixed types but one branch is unavailable and therefore has no downstream impact assuming it stays unavailable.
| @warn "Normalizing mixed parallel Line/DiscreteControlledACBranch on near-zero-impedance arc $(arc_key[1])-$(arc_key[2]) to DiscreteControlledACBranch." _group = | ||
| IS.LOG_GROUP_PARSING maxlog = PS_MAX_LOG | ||
| else | ||
| @warn "Keeping mixed parallel Line/DiscreteControlledACBranch on arc $(arc_key[1])-$(arc_key[2]) because at least one Line has non-negligible impedance." _group = |
There was a problem hiding this comment.
Couldn't we theoretically get to this last else with some other combination of branch types in the case of bad data? Might make sense to at lease include the branch types in the log message instead of just "Line/DiscreteControlledACBranch".
Thanks for opening a PR to PowerSystems.jl, please take note of the following when making a PR:
Check the contributor guidelines
Description
The GitHub Actions
build(docs) job was failing with the following error:check_parallel_branch_type_consistencyis a public exported function with a docstring insrc/utils/IO/system_checks.jl, but it was not referenced in any@docsor@autodocsblock in the documentation. Documenter.jl's:missing_docscheck treats this as a build error.Fix: Added a
@docs check_parallel_branch_type_consistencyblock todocs/src/api/public.mdin the System section so the docstring is included in the public API reference.