Pandapower converter: fix 3-Ph transformer loading Calculation

[furqan463]

Fixes #319
Fixes #321

Changes proposed in this PR include:

Extended "u1" and "u2" arrays from (n,) dimensions to (n,3) dimensions to remove value error.
multiplied phase loading percent with sqrt(3), (i * v / sqrt(3)) / (sn / 3) --> sqrt(3) * (i * v) / sn
divided total loading percent with sqrt(3) as v_ph = v / sqrt(3).

Electrical Mathematics:

Power per Phase = V_n * I_line_to_line # Basic Definition
Capacity per Phase = Sn / 3
Loading per Phase = Power per Phase / Capacity per Phase

ui = i_line_to_line * V_line_to_line
Loading per Phase = [ui / sqrt(3)] / [Sn / 3]
= [3 / sqrt(3)] * [ui] / [Sn]
= sqrt(3) * ui / Sn

Total Power = sum[Power_Phase_a, Power_Phase_b, Power_Phase_c]
= sum[ ui_a / sqrt(3), ui_b / sqrt(3), ui_c / sqrt(3)]
= [ sum(ui_a, ui_b, ui_c) / sqrt(3) ]
Total Loading = [sum(ui_a, ui_b, ui_c) / sqrt(3)] / Sn

Looking at it from another perspective which is more relevant in case of trafo_loading="current"
loading_per_phase = phase_current(line_to_line) / rated_current
rated_current (I_rated) = Sn / (sqrt(3) x V_line_to_line) #As for 3-Ph power system S = sqrt(3) * V * I
loading_per_phase = I_ph / I_rated
= I_ph / [Sn / (sqrt(3) x V_line_to_line)]
= sqrt(3) * [I_ph * V_line_to_line] / Sn

Checks before merge

• Review the mathematics
• Reproduce; verify that this fixes the issue
• Code review
• Add tests
• Pass CI (Sonar Cloud may fail with HTML error code 403. That is not a problem with this PR)

[mgovers]

Thank you for your contribution @furqan463 ! We greatly appreciate you proposing the fix to the bug you found.

This is just a code review. Still TODO:

• add some tests
• review electrical engineering correctness
• reproduce with a pandapower grid.

[nitbharambe]

Nice spotting the bug!
For reference, as given in pandapower documentation:

As in their code:

https://github.com/e2nIEE/pandapower/blob/7b7ab33a8a9cc1b9802e168dfeea824eeb2ef266/pandapower/results_branch.py#L461-L465

The description and these two definitions are all different I suppose. (Atleast the naming is different :D) Makes it quite confusing.

[furqan463]

@mgovers I've updated the description above elaborating mathematics involved. Moreover, thanks to @nitbharambe proposed changes match with existing code in PandaPower. I'm attaching a small code to reproduce the errors as well as testing the proposed changes. I'm trying to create unit tests, but while working on it, I've found some bugs with existing validation data.
pgm_test.py

[furqan463]

@mgovers I've highlighted in relevant test code, why the unit test was not working for this function.
I'm trying to correct this unit test as well.

[furqan463]

@mgovers I've updated the validation unittest, now the test includes all components and since I loading pp_input to converter, all components are being converted and tested.
Now the for trafo_3ph, loading test will fail on original code for all 4 cases. However with amended code of pandapower_converter under trafo_loading=="current", loading for a & b phase will pass but for c and total will still fail.
Upon investigation I found out that the trafo loading data in validation files is for the case trafo_loading=="power", however, I found that the code for trafo_loading=="power" in pandapower_converter was also wrong, which now I corrected and amended unittest function to use trafo_loading="power", and now all test are passing.
However, for trafo_loading="current", we still need to generate validation data.

[mgovers]

Hi @furqan463,

Nice find again! I am thinking that maybe we can try to add your validation case as a validation case similar to

import pandapower as pp
import numpy as np
from math import sqrt


def create_net():
    net = pp.create_empty_network('test', f_hz=50)
    pp.create_bus(net, 11, 'source')
    pp.create_bus(net, 11, 'TF_HT')
    pp.create_bus(net, 0.4, 'TF_LT')

    pp.create_ext_grid(net, 0, 1.0, 0, s_sc_max_mva=100, rx_max = 0.1, x0x_max=1, r0x0_max=0.1)
    pp.create_line_from_parameters(net, 0, 1, 2, 0.33, 0.34,
                                0.001, 600, r0_ohm_per_km=0.66, x0_ohm_per_km=0.65, c0_nf_per_km=0.001,
                                g_us_per_km=0, g0_us_per_km=0)
    pp.create_transformer_from_parameters(net, 1,2, 1, 11, 0.415, 0,
                                        4, 0, 0.01, -30, vector_group='Dyn',
                                        vk0_percent=4, vkr0_percent=0, mag0_percent=100, mag0_rx=0, si0_hv_partial=0.9)
    pp.create_asymmetric_load(net, 2, 0.2, 0.19, 0.195,
                            0.05, 0.045, 0.035, 0, type='wye')
    return net


def check_result(net):
    v_hv = net.trafo.vn_hv_kv
    v_lv = net.trafo.vn_lv_kv
    i_max_hv = np.divide(net.trafo.sn_mva, v_hv* np.sqrt(3)) * 1e3
    i_max_lv = np.divide(net.trafo.sn_mva, v_lv* np.sqrt(3)) * 1e3


    i_a_hv = net.res_trafo_3ph.loc[:, 'i_a_hv_ka'] * 1000
    i_b_hv = net.res_trafo_3ph.loc[:, 'i_b_hv_ka'] * 1000
    i_c_hv = net.res_trafo_3ph.loc[:, 'i_c_hv_ka'] * 1000

    i_a_lv = net.res_trafo_3ph.loc[:, 'i_a_lv_ka'] * 1000
    i_b_lv = net.res_trafo_3ph.loc[:, 'i_b_lv_ka'] * 1000
    i_c_lv = net.res_trafo_3ph.loc[:, 'i_c_lv_ka'] * 1000

    np.testing.assert_allclose(np.maximum(i_a_hv / i_max_hv, i_a_lv / i_max_lv)*100, net.res_trafo_3ph.loading_a_percent)
    np.testing.assert_allclose(np.maximum(i_b_hv / i_max_hv, i_b_lv / i_max_lv)*100, net.res_trafo_3ph.loading_b_percent)
    np.testing.assert_allclose(np.maximum(i_c_hv / i_max_hv, i_c_lv / i_max_lv)*100, net.res_trafo_3ph.loading_c_percent)


def compare_result(actual, expected, *, rtol):
    np.testing.assert_allclose(actual.trafo.vn_hv_kv, expected.trafo.vn_hv_kv, rtol=rtol)
    np.testing.assert_allclose(actual.trafo.vn_lv_kv, expected.trafo.vn_lv_kv, rtol=rtol)
    np.testing.assert_allclose(actual.trafo.sn_mva, expected.trafo.sn_mva, rtol=rtol)
    np.testing.assert_allclose(actual.res_trafo_3ph.loc[:, 'i_a_hv_ka'], expected.res_trafo_3ph.loc[:, 'i_a_hv_ka'], rtol=rtol)
    np.testing.assert_allclose(actual.res_trafo_3ph.loc[:, 'i_b_hv_ka'], expected.res_trafo_3ph.loc[:, 'i_b_hv_ka'], rtol=rtol)
    np.testing.assert_allclose(actual.res_trafo_3ph.loc[:, 'i_c_hv_ka'], expected.res_trafo_3ph.loc[:, 'i_c_hv_ka'], rtol=rtol)
    np.testing.assert_allclose(actual.res_trafo_3ph.loc[:, 'i_a_lv_ka'], expected.res_trafo_3ph.loc[:, 'i_a_lv_ka'], rtol=rtol)
    np.testing.assert_allclose(actual.res_trafo_3ph.loc[:, 'i_b_lv_ka'], expected.res_trafo_3ph.loc[:, 'i_b_lv_ka'], rtol=rtol)
    np.testing.assert_allclose(actual.res_trafo_3ph.loc[:, 'i_c_lv_ka'], expected.res_trafo_3ph.loc[:, 'i_c_lv_ka'], rtol=rtol)
    np.testing.assert_allclose(actual.res_trafo_3ph.loading_a_percent, expected.res_trafo_3ph.loading_a_percent, rtol=rtol)
    np.testing.assert_allclose(actual.res_trafo_3ph.loading_b_percent, expected.res_trafo_3ph.loading_b_percent, rtol=rtol)
    np.testing.assert_allclose(actual.res_trafo_3ph.loading_c_percent, expected.res_trafo_3ph.loading_c_percent, rtol=rtol)


def test_pgm_pp_converter():
    pgm_net = create_net()
    pp_net = create_net()
    pp.runpp_pgm(pgm_net, symmetric=False)
    pp.runpp_3ph(pp_net)
    check_result(pgm_net)
    check_result(pp_net)
    compare_result(pgm_net, pp_net, rtol=0.04)


if __name__ == "__main__":
    test_pgm_pp_converter()

The current main has huge differences, while your branch passes, so your PR definitely solves the problem.

That said, I do still see some small differences between PGM and PP if I run that script (the compare_result step) of up to 4 percent (see the rtol), particularly in the b phase. However, this does not seem to be caused by the incorrect loading calculation, as the hand calculation seems to match for both calculation backends. Instead, the other result values seem to differ, but that could be caused by different local minima. I did not dig any further into this.

[mgovers]

So far, it seems like you're solving the issues nicely, so I don't think my help is needed as of now. If you do need some input from me, I'm happy to help.

[mgovers]

@furqan463 how is it going with the tests?

Since you found a bug and have already resolved it, I'm also OK with merging in its current state without extensive tests (after a minor clean-up). The tests can then be added as a follow-up. The reasoning behind that is that users can already use the PGM-IO with the bugfix.

What are your thoughts?

[furqan463]

@mgovers agreed. I couldn't find time to finalise the tests, but agreed we can merge without tests, as the changes are broadly conceptual.

[furqan463]

@mgovers also added fix for issue #321 in this commit.

[mgovers]

@mgovers also added fix for issue #321 in this commit.

Awesome! I've linked the issue in this PR, so it will automatically close that issue.

Let's also add a separate test case for this

@mgovers I see you've added the minimal test and it's working with rtol=0.04 but there's a problem with this test and the reason why I did not make a validation test to compare pp and pgm results. In fact the reason I turned to pgm for 3ph and happen to find this bug is because PandaPower do not properly handle Dyn transformer in unbalanced condition. All it does is remove zero sequence from load, while actually hv current of each phase is vector difference of lv currents of two related phases. In this case the imbalance in the load is minimal so it's working with rtol=0.04, if we use balanced load, the test passes with rtol=0.004 as well which I'm going to do in next commit. But conceptually it's not right. We cannot validate pgm results against pp results in unbalanced 3ph analysis with delta-start transformation. And lastly in my opinion comparison of results of pp and pgm is not in the scope of converter.

It's great to hear that you're switching to PGM as a result of this improved handling. It's a type of user story that really shows we're helping out the community with this project (c.c. @petersalemink95).

Regarding the scope of the tests in this repo, I do agree with you, but I'm a little bit hesitant to completely remove the test as well.

• On one hand, we do want to compare the two (as long as the difference is explained, it serves as a sanity check/validation: PP and PGM should be similar but not identical).
• On the other, you're completely right in that validation of the method itself should be out of scope of this repo. However, there's no way to verify that the conversion was actually correct except for doing a full end-to-end validation.

The good thing is that we have both tests/unit and tests/validation subdirectories. As a resolution to the above contradiction, the tests/unit directory is reserved for tests regarding the scope of this repo (conversion only), while the tests/validation directory is reserved for end-to-end tests. We have had issues with pandapower changing the modeling and therefore also the outcome when it switched from pandapower 2.x to pandapower 3.x, so it is possible that the tests will break in the future as well, but at least, we'll be notified and can act accordingly when it happens.

Regarding the unbalanced v.s. balanced load tests, I think both have their own places in the validation tests: One to prove that balanced works correctly, and one to prove that unbalanced works similarly between PGM and PP, but not exactly the same (to be added). They can be 2 separate tests with 2 separate fixtures.

@nitbharambe do you maybe have any good ideas regarding this?

[mgovers]

hi @furqan463,

Since you are actively picking up this PR again, the team has decided to treat this as a regular external contribution PR again and only merge it once all testing is fully in place.

Please let me know if you need help - if not, I'll be on stand-by.

[nitbharambe]

@mgovers I see you've added the minimal test and it's working with rtol=0.04 but there's a problem with this test and the reason why I did not make a validation test to compare pp and pgm results. In fact the reason I turned to pgm for 3ph and happen to find this bug is because PandaPower do not properly handle Dyn transformer in unbalanced condition. All it does is remove zero sequence from load, while actually hv current of each phase is vector difference of lv currents of two related phases. In this case the imbalance in the load is minimal so it's working with rtol=0.04, if we use balanced load, the test passes with rtol=0.004 as well which I'm going to do in next commit. But conceptually it's not right. We cannot validate pgm results against pp results in unbalanced 3ph analysis with delta-start transformation. And lastly in my opinion comparison of results of pp and pgm is not in the scope of converter.

On one hand, we do want to compare the two (as long as the difference is explained, it serves as a sanity check/validation: PP and PGM should be similar but not identical).

so it is possible that the tests will break in the future as well, but at least, we'll be notified and can act accordingly when it happens.

Agreed with these! The validation test here and rtols should not be used for modelling comparison. Hence, we should be less stringent on the rtol setting here if test fails because of it (Now or in future.)

We had earlier also thought of having a default vs custom rtol for specific attribute, but that was ruled out. We can do it if more such cases occur (Not because it would enable finer validation, but to clean up code.).

[mgovers]

@furqan463 looks quite good. A couple small remarks left, but I think the main piece of the code is pretty much finished.

[furqan463]

Functions pp_trafo_output and pp_switch_output not being tested for the same reason as was for pp_trafo_output_3ph, the functions are returning from first if because pp_data is not passed to converter. These two functions are also appearing missed in coverage, resulting in coverage of only 89% for this file. Now trying to fix this.

[furqan463]

@mgovers I'm done now. There's one unittest failing that I'm not sure how to resolve. As I understand the test check exactly how we calculate "tan1" in pgm_input_line, but I changed the formula to avoid Nan values.

[mgovers]

@mgovers I'm done now. There's one unittest failing that I'm not sure how to resolve. As I understand the test check exactly how we calculate "tan1" in pgm_input_line, but I changed the formula to avoid Nan values.

Awesome, @furqan463 , I'll have a look when i can 👍

[mgovers]

@nitbharambe looks like this PR is now in a good-to-go state. Since I've also contributed significantly, can you please review?

[mgovers]

OH apparently, CI is still failing; investigating.

[mgovers]

found the reason for the remaining validation tests.

To run the validation tests locally, please either use a test extension or run pytest tests/validation rather than just pytest, as the validation tests are excluded from pytest by default.

[furqan463]

@mgovers please check now, I've updated the data file.
TODOs added for future.

[mgovers]

@mgovers please check now, I've updated the data file. TODOs added for future.

Great! @nitbharambe and I will have a look on Monday.

• concerning the license file for the json data, you can copy-paste one of the other files such that foo.json has a file foo.json.license. The .license files should be the same across the codebase.
• please install the dev dependencies and run pre-commit run --all-files to run the formatter, linter and type checker. It will also run a check that checks whether you added the license file correctly.