Conversation
|
Fixes #294 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR removes the type-parameterization from BranchesParallel, updating constructors, helper methods, mapping utilities, and a test to work with a non-generic BranchesParallel container.
Changes:
- Refactors
BranchesParallel{T}into a non-parameterizedBranchesParallelholdingVector{PSY.ACTransmission}. - Updates Ybus parallel-branch insertion logic to operate on the non-parameterized structure.
- Adjusts network reduction typed-map utilities and tests to expect
BranchesParallelrather thanBranchesParallel{T}.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| test/test_arc_types_and_reductions.jl | Updates reduction-type assertion to use BranchesParallel without a type parameter. |
| src/Ybus.jl | Refactors _push_parallel_branch! to accept PSY.ACTransmission and handle mixed-type parallels without re-wrapping. |
| src/NetworkReductionData.jl | Updates typed parallel-branch map return types and segment helpers for non-parameterized BranchesParallel. |
| src/BranchesParallel.jl | Implements non-parameterized BranchesParallel and updates constructors, map-filtering logic, and helpers accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| br::PSY.ACTransmission, | ||
| ) | ||
| bp = parallel_branch_map[arc_tuple] | ||
| if any(typeof(b) != typeof(br) for b in bp.branches) |
There was a problem hiding this comment.
This warning will trigger on every subsequent insertion once a circuit becomes mixed-type (and may also warn repeatedly even when adding a branch of an already-present type). To reduce log noise, warn only when the newly added branch introduces a new concrete type (e.g., compare typeof(br) against the existing set of types), or record that a given arc_tuple has already emitted the mismatch warning.
| if any(typeof(b) != typeof(br) for b in bp.branches) | |
| existing_branch_types = Set(typeof(b) for b in bp.branches) | |
| if !isempty(existing_branch_types) && !(typeof(br) in existing_branch_types) |
| mutable struct BranchesParallel <: PSY.ACTransmission | ||
| branches::Vector{PSY.ACTransmission} |
There was a problem hiding this comment.
Storing branches as Vector{PSY.ACTransmission} (an abstract element type) can significantly increase dynamic dispatch and reduce inference in performance-sensitive loops (e.g., building Ybus / iterating branch properties). If this code is on a hot path, consider preserving concreteness by keeping a typed representation internally (e.g., continuing to store homogeneous parallels in a concrete vector and only type-erasing for truly mixed-type collections), or adding a fast-path for the common homogeneous case to avoid repeatedly operating on abstractly-typed elements.
| function BranchesParallel(branches::Vector{<:PSY.ACTransmission}) | ||
| BranchesParallel(Vector{PSY.ACTransmission}(branches), nothing) |
There was a problem hiding this comment.
Storing branches as Vector{PSY.ACTransmission} (an abstract element type) can significantly increase dynamic dispatch and reduce inference in performance-sensitive loops (e.g., building Ybus / iterating branch properties). If this code is on a hot path, consider preserving concreteness by keeping a typed representation internally (e.g., continuing to store homogeneous parallels in a concrete vector and only type-erasing for truly mixed-type collections), or adding a fast-path for the common homogeneous case to avoid repeatedly operating on abstractly-typed elements.
| filters::Dict, | ||
| ) where {T <: PSY.ACTransmission} | ||
| function add_to_map(double_circuit::BranchesParallel, filters::Dict) | ||
| isempty(filters) && return true |
There was a problem hiding this comment.
only(branch_types) will throw if double_circuit.branches is empty (since branch_types would be empty). If BranchesParallel can ever be constructed/observed with an empty branches vector, add an explicit guard (e.g., return true or throw a clearer error) before calling only.
| isempty(filters) && return true | |
| isempty(filters) && return true | |
| isempty(double_circuit.branches) && return true |
| T = only(branch_types) | ||
| if !haskey(filters, T) |
There was a problem hiding this comment.
only(branch_types) will throw if double_circuit.branches is empty (since branch_types would be empty). If BranchesParallel can ever be constructed/observed with an empty branches vector, add an explicit guard (e.g., return true or throw a clearer error) before calling only.
| ::Type{T}, | ||
| ) where {T <: PSY.ACTransmission} | ||
| return b.parallel_branch_map[T]::Dict{Tuple{Int, Int}, BranchesParallel{T}} | ||
| return b.parallel_branch_map[T]::Dict{Tuple{Int, Int}, BranchesParallel} |
There was a problem hiding this comment.
The function name get_typed_parallel_branch_map suggests the returned map is typed/consistent w.r.t. T, but the value type is now BranchesParallel (which can contain mixed ACTransmission types). Consider either (a) renaming/adjusting the API/docs to avoid implying type-homogeneity, or (b) enforcing (via checks) that entries in parallel_branch_map[T] only contain branches of type T when returned through this accessor.
Performance ResultsPrecompile Time
Execution Time
|
|
This is currently failing in the model build for WECC as well. |
No description provided.