-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Jd/radial branch reduction #1033
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@GabrielKS with the correct branches |
@GabrielKS I think that now with the doctoring change. The code should be ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. There are other updates here on top of the radial branches. I think the thermal multistart and bump some tests to power models are here too.
Thanks Gabriel for the help here too. It seems that some code cleaning and standarization is required for later, especially with the PowerModels update that made it more visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A typo and a suggestion to use nothing
rather than an empty dictionary to indicate a null value.
) | ||
time_steps = get_time_steps(container) | ||
ac_bus_numbers = collect(Iterators.flatten(values(subnetworks))) | ||
if isempty(bus_reduction_map) | ||
ac_bus_numbers = collect(Iterators.flatten(values(subnetworks))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand what bus_reduction_map
is, but I'm a little wary of a completely different thing happening when it is empty. If an empty dictionary is meant to be a "null value" signifying that the bus reduction map should not be used, maybe making it nothing
(and editing the relevant zero-argument constructor, etc. accordingly) would be a better way to indicate this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bus reduction map keep track of where the components of a bus that has been eliminated a now connected. For instance if a bus A is radially connected to B and A was eliminated, what the map does is to establish the model won't include A and anything connected to A will be connected to B in the model
@@ -37,7 +37,13 @@ function add_variables!( | |||
U <: PM.AbstractActivePowerModel, | |||
} | |||
time_steps = get_time_steps(container) | |||
bus_numbers = PSY.get_number.(get_available_components(PSY.ACBus, sys)) | |||
radial_network_reduction = get_radial_network_reduction(network_model) | |||
if isempty(radial_network_reduction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above wrt using empty dictionary as null value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point. We don't have a general rule to handle theses cases and generally have preferred to use empty objects than nothing values. I don't think it is too much of a problem at this stage in the code.
pm_data["nw"]["$(t)"]["bus"]["$(bus.number)"]["inj_p"] = | ||
network_model = get_network_model(template) | ||
radial_network_reduction = get_radial_network_reduction(network_model) | ||
if isempty(radial_network_reduction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above wrt empty dict as null
Performance Results
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1033 +/- ##
==========================================
+ Coverage 80.42% 80.56% +0.14%
==========================================
Files 116 116
Lines 12423 12531 +108
==========================================
+ Hits 9991 10096 +105
- Misses 2432 2435 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, pulling out the null values discussion into #1045 to resolve later.
Requires the branch afc/radial_branches_matrices in PowerNetworkMatrices
Requires
NREL-Sienna/PowerNetworkMatrices.jl#69
NREL-Sienna/PowerSystemCaseBuilder.jl#69