Corner cases catches #70
Conversation
|
Performance Results
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
==========================================
- Coverage 78.02% 77.93% -0.09%
==========================================
Files 15 15
Lines 1388 1396 +8
==========================================
+ Hits 1083 1088 +5
- Misses 305 308 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| subnetworks::Dict{Int, Set{Int}}, | ||
| ref_bus_positions::Set{Int}, | ||
| ref_buses::Vector{Int}, |
There was a problem hiding this comment.
Why is this now a Vector rather than a Set?
There was a problem hiding this comment.
because before it was incorrectly using the ref buses positions and not the ref buses values.
| # system with small islands that represent larger interconnected areas. | ||
| if length(SparseArrays.nzrange(A, new_parent_val)) < 2 | ||
| @warn "Bus $parent_bus_number Parent $new_parent_bus_number is a leaf node. Indicating there is an island." | ||
| push!(bus_reduction_map_index[parent_bus_number], new_parent_bus_number) |
There was a problem hiding this comment.
Would it be possible to get a test case of the element of the public interface that depends on this that fails without this change and passes with it? Nothing fails when I comment this line out.
There was a problem hiding this comment.
or a # TODO or issue if this is too complicated to do on the current timeframe
There was a problem hiding this comment.
Codecov concurs and points out a few other untested regions
There was a problem hiding this comment.
I will create an issue to implement the test. This is a corner case catch.
| ) | ||
| if j ∈ ref_bus_positions |
There was a problem hiding this comment.
I don't really understand why we're doing this, but that may be due to my lack of familiarity with PNM
There was a problem hiding this comment.
We never want to reduce the reference buses. It creates issues depending on the model once we remove tha bus that is meant to absorb all the power differences.
| sbn, ref_bus_positions = _find_subnetworks(sys) | ||
| return assign_reference_buses(sbn, ref_bus_positions) | ||
| sbn, ref_buses = _find_subnetworks(sys) | ||
| return assign_reference_buses!(sbn, ref_buses) |
There was a problem hiding this comment.
All the other existing assign_reference_buses calls have been edited to add a bus_ax_ref — this one doesn't need one?
There was a problem hiding this comment.
in some cases we already have the references buses so there is no need to recalculate them based on the ref_ax and location.
| @@ -336,6 +333,15 @@ function assign_reference_buses( | |||
| return bus_groups | |||
| end | |||
|
|
|||
| function assign_reference_buses!( | |||
There was a problem hiding this comment.
Ideally we'd get a test case that demonstrates what breaks without this new layer of indirection — if too complicated to do now but potentially feasible in the future, add a # TODO or GitHub Issue
|
Added #71 for the tests |
This PR addresses some bugs to catch corner cases exposed by the NTP data set.