Skip to content
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

Merged
merged 82 commits into from
Jan 22, 2024
Merged

Jd/radial branch reduction #1033

merged 82 commits into from
Jan 22, 2024

Conversation

jd-lara
Copy link
Member

@jd-lara jd-lara commented Jan 2, 2024

Requires the branch afc/radial_branches_matrices in PowerNetworkMatrices

Requires

NREL-Sienna/PowerNetworkMatrices.jl#69
NREL-Sienna/PowerSystemCaseBuilder.jl#69

src/core/optimization_container.jl Outdated Show resolved Hide resolved
src/core/optimization_container.jl Outdated Show resolved Hide resolved
src/core/optimization_container.jl Outdated Show resolved Hide resolved
src/core/optimization_container.jl Outdated Show resolved Hide resolved
src/core/optimization_container.jl Outdated Show resolved Hide resolved
@jd-lara jd-lara requested a review from rodrigomha January 2, 2024 21:53
@jd-lara jd-lara self-assigned this Jan 2, 2024
@jd-lara jd-lara marked this pull request as ready for review January 3, 2024 17:39
test/test_radial_branches.jl Outdated Show resolved Hide resolved
src/core/network_model.jl Outdated Show resolved Hide resolved
src/network_models/powermodels_interface.jl Outdated Show resolved Hide resolved
test/test_network_constructors.jl Outdated Show resolved Hide resolved
test/test_network_constructors.jl Outdated Show resolved Hide resolved
test/test_network_constructors.jl Outdated Show resolved Hide resolved
@jd-lara
Copy link
Member Author

jd-lara commented Jan 15, 2024

@GabrielKS with the correct branches
Uploading image.png…

@jd-lara
Copy link
Member Author

jd-lara commented Jan 16, 2024

@GabrielKS I think that now with the doctoring change. The code should be ready.

Copy link
Contributor

@rodrigomha rodrigomha left a 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.

@jd-lara jd-lara requested a review from GabrielKS January 22, 2024 15:44
Copy link
Contributor

@GabrielKS GabrielKS left a 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.

src/core/network_model.jl Outdated Show resolved Hide resolved
src/core/network_model.jl Outdated Show resolved Hide resolved
)
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)))
Copy link
Contributor

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?

Copy link
Member Author

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

src/core/optimization_container.jl Show resolved Hide resolved
@@ -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)
Copy link
Contributor

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

Copy link
Member Author

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)
Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Jan 22, 2024

Performance Results

Version Precompile Time
Main 3.29603466
This Branch 3.272836088
Version Build Time
Main-Build Time Precompile 51.549357009
Main-Build Time Postcompile 2.644818698
This Branch-Build Time Precompile 52.581710687
This Branch-Build Time Postcompile 2.782183066

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (003dbb0) 80.42% compared to head (782e017) 80.56%.
Report is 23 commits behind head on main.

❗ Current head 782e017 differs from pull request most recent head f93f740. Consider uploading reports for the commit f93f740 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
unittests 80.56% <93.41%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/core/definitions.jl 68.75% <ø> (ø)
src/core/optimization_container.jl 79.08% <100.00%> (+0.18%) ⬆️
...s_models/device_constructors/branch_constructor.jl 80.20% <100.00%> (ø)
src/initial_conditions/initialization.jl 96.42% <100.00%> (+0.06%) ⬆️
src/network_models/network_constructor.jl 82.85% <100.00%> (+6.66%) ⬆️
src/network_models/network_slack_variables.jl 100.00% <100.00%> (ø)
src/network_models/pm_translator.jl 81.13% <100.00%> (+0.93%) ⬆️
src/network_models/powermodels_interface.jl 87.40% <100.00%> (+0.38%) ⬆️
src/operation/decision_model.jl 92.50% <100.00%> (+0.04%) ⬆️
src/operation/decision_model_store.jl 96.61% <ø> (ø)
... and 6 more

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@jd-lara jd-lara merged commit dbe34bd into main Jan 22, 2024
7 checks passed
@jd-lara jd-lara deleted the jd/radial_branch_reduction branch January 22, 2024 22:31
Copy link
Contributor

@GabrielKS GabrielKS left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants