wip: loss factor variable #1238
Conversation
|
Performance Results
|
Co-authored-by: Gabriel Konar-Steenberg <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Gabriel Konar-Steenberg <[email protected]>
There was a problem hiding this comment.
Looks pretty good, left a few comments. If this has a dependency on NREL-Sienna/PowerFlows.jl#98 or other PRs, can you note that in the PR description? Also can you add a description of what this PR does there for future reference?
test/test_simulation_results.jl
Outdated
| # here we check if the loss factors are stored in the results, the values are tested in PowerFlows.jl | ||
| @test loss_factors !== nothing | ||
|
|
||
| loss_factors = first( |
There was a problem hiding this comment.
why are you getting the loss_factors two different ways?
There was a problem hiding this comment.
Just to make sure it works. Should I remove one of them?
There was a problem hiding this comment.
Yeah, I would say that the correspondence between read_aux_variable and read_results_with_keys is/ought to be tested elsewhere, so you should only need one of them
test/test_simulation_results.jl
Outdated
|
|
||
| # here we check if the loss factors are stored in the results, the values are tested in PowerFlows.jl | ||
| @test loss_factors !== nothing | ||
| @test nrow(loss_factors) == 576 |
There was a problem hiding this comment.
Where does 576 come from? Can we do ... == length(get_components(... or something instead to avoid hard-coding this?
There was a problem hiding this comment.
It is 48 * 12 (see also line 1067). Do you have an idea where it is best to read it from?
There was a problem hiding this comment.
It is coming from the file used ("export_48_12") so expressing it as 48 * 12 should be ok
…s.jl into rb/loss_factors_variable
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
test/test_simulation_results.jl
Outdated
| # here we check if the loss factors are stored in the results, the values are tested in PowerFlows.jl | ||
| @test loss_factors !== nothing | ||
|
|
||
| loss_factors = first( |
There was a problem hiding this comment.
Yeah, I would say that the correspondence between read_aux_variable and read_results_with_keys is/ought to be tested elsewhere, so you should only need one of them
test/test_simulation_results.jl
Outdated
|
|
||
| # here we check if the loss factors are stored in the results, the values are tested in PowerFlows.jl | ||
| @test loss_factors !== nothing | ||
| @test nrow(loss_factors) == 576 |
Co-authored-by: Gabriel Konar-Steenberg <[email protected]>
There was a problem hiding this comment.
Looks good from a code quality standpoint. Ideally we'd release https://github.com/NREL-Sienna/PowerFlows.jl/pull/98/files and make sure the tests pass on the CI for this before merging it, but I'll let @jd-lara manage
Here we add the reading of the loss factors after a successful AC power flow calculation in
PowerFlowsas a new auxiliary variablePowerFlowLossFactors.PowerFlowLossFactorsPowerFlowData(only ifcalc_loss_factorswas set to true in ACPowerFlow)calc_loss_factorsistrueand that it is not present when it isfalse. The values of loss factors are tested only inPowerFlowsThis PR depends on NREL-Sienna/PowerFlows.jl#98