Step-up transformer tap changer support: additional tests #868
Conversation
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
|
Could you update the naming etc as per agreement? https://power-grid-model.readthedocs.io/en/stable/contribution/CONTRIBUTING.html#pull-request-process |
figueroa1395
changed the title
Thanks for the reminder, I had forgotten. Fixed accordingly. |
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
|
AKI's use case was this way: source -- (HV)transformer(MV) -- (MV)transformer(higher MV with tap and control side) -- load Can you also create a case? Such that source are at other place than control and tap_side? |
tests/data/power_flow/step-up-transformer/max-voltage-tap/input.json
Outdated
Show resolved
Hide resolved
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
There was a problem hiding this comment.
- The
step-up-transformer-singleshould not live undertests/data/power_flow/directly, instead, the content should be arranged like all other cases intests/data/power_flow/automatic_tap_regulatorand havestep-up-transformer-singleprefixing all cases - Could you explain to me how is the validation case in the input json files constructed? You put the control side at the other side, but you still have high voltage at from side
| @@ -184,7 +184,7 @@ TEST_CASE("Test Transformer ranking") { | |||
| CHECK_NOTHROW(pgm_tap::build_transformer_graph(get_state(6, 2, 1, 4, 5))); | |||
| } | |||
|
|
|||
| SUBCASE("Full grid") { | |||
| SUBCASE("Full grid 1") { | |||
There was a problem hiding this comment.
Could you rename the test case to be more descriptive like "Full radial grid" etc
There was a problem hiding this comment.
Addressed. Both grids 1 and 2 are really just full meshed grids, so I tried to add a bit more description to the name. If you have another suggestion, just let me know. Maybe some in code comments can be added as well if deemed necessary.
| CHECK_THROWS_AS(pgm_tap::rank_transformers(state), AutomaticTapInputError); | ||
| } | ||
| } | ||
|
|
||
| // The test grid 2 is compatible with the updated logic for step up transformers | ||
| SUBCASE("Full grid 2") { |
There was a problem hiding this comment.
Could you rename the test case to be more descriptive like "Full meshed grid" etc
There was a problem hiding this comment.
Addressed. See above.
| // =====Test Grid===== | ||
| // ________[0]________ | ||
| // || | | | ||
| // [1] [4]--[5] | ||
| // | | | | ||
| // [2] | [8] | ||
| // | [6] | | ||
| // [3]----[7]---| [9] | ||
| // | | | ||
| // L--------------[10] | ||
| TestState state; | ||
| std::vector<NodeInput> nodes{{0, 150e3}, {1, 10e3}, {2, 10e3}, {3, 10e3}, {4, 10e3}, {5, 50e3}, | ||
| {6, 10e3}, {7, 10e3}, {8, 10e3}, {9, 10e3}, {10, 10e3}}; | ||
| main_core::add_component<Node>(state, nodes.begin(), nodes.end(), 50.0); | ||
|
|
||
| std::vector<TransformerInput> transformers{ | ||
| get_transformer(11, 0, 1, BranchSide::from), get_transformer(12, 0, 1, BranchSide::from), | ||
| get_transformer(13, 2, 3, BranchSide::from), get_transformer(14, 6, 7, BranchSide::from), | ||
| get_transformer(15, 5, 8, BranchSide::from), get_transformer(16, 9, 10, BranchSide::from)}; | ||
| main_core::add_component<Transformer>(state, transformers.begin(), transformers.end(), 50.0); |
There was a problem hiding this comment.
What is the purpose of this test grid?
There was a problem hiding this comment.
With the introduction of the new logic, the old grid was no longer valid for the transformer ranking algorithm, as it was pointed out by you in b642c97. Hence, the old grid was left in since it was still valid for the other parts of the "full" algorithm, while marking the transformer ranking part with CHECK_THROWS_AS(pgm_tap::rank_transformers(state), AutomaticTapInputError) in d947149; whereas the new grid is only used for the ranking itself.
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Okay, addressed. In addition, given your other question, I have renamed those test with the prefix
To my understanding, previously it was forbidden to have the control side on the HV side, however, with the change of logic, it can now be positioned at the LV side. Hence, the current validation cases try to reflect this. On the contrary, this validation case doesn't reflect the specific edge case that AKI ran into, pointed out by you and @nitbharambe as well. For this reason, I am working in an additional validation case, which will include not only the control side at the HV (relative) side, but also away from the source. These set of tests are to be prefixed Until I add the missing validation test, I am putting back this PR into draft. |
|
Add missing tests and refactor inappropriate ones for (#826):