Refactoring voltage control #168

flo-dup · 2020-11-10T14:40:05Z

Please check if the PR fulfills these requirements (please use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Does this PR already have an issue describing the problem ? If so, link to this issue using '#XXX' and skip the rest
No

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Refactoring voltage control to have a structure similar to PhaseDiscreteControl / VoltageDiscreteControl, that is, a "control" object which makes the link between the controller and the controllee.

annetill

I have left the review of abstract lf bus and lf network loader impl for latter. I think that we have to review bases before in having a unique voltage control in case of remote shared control but split it in several voltage control if remote voltage control is desactivated. This solution is not clear enough to be prefer at the current one.

src/main/java/com/powsybl/openloadflow/ac/ReactiveLimitsOuterLoop.java

src/main/java/com/powsybl/openloadflow/network/LfBus.java

src/main/java/com/powsybl/openloadflow/network/VoltageControl.java

src/main/java/com/powsybl/openloadflow/network/impl/AbstractLfGenerator.java

src/main/java/com/powsybl/openloadflow/network/impl/LfDanglingLineGenerator.java

src/main/java/com/powsybl/openloadflow/network/impl/LfGeneratorImpl.java

src/main/java/com/powsybl/openloadflow/network/impl/LfStaticVarCompensatorImpl.java

src/main/java/com/powsybl/openloadflow/network/impl/LfVscConverterStationImpl.java

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>

First import of VoltageControl. Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Remove the HashMap previously used Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Done in createVoltageControls, together with various consistency checks Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

AcEquationSystemCreationParameters.voltageRemoteControl now useless thus removed Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>

Some tests were not included in build check as they were annotated with org.junit.api.Test Code smells corrections Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Nothing done in AbstractLfBus anymore, now in: - AbstractLfGenerator when setting targetV - LfNetworkLoaderImpl when creating controller - controllee links Unit tests are slightly changed as a consequence Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Instead of controllersBus list / controlledBus optional Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

sonarqubecloud · 2020-11-19T15:04:29Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
3 Code Smells

91.2% Coverage
0.0% Duplication

annetill

Some small changes. After @geofjamg review, I will perform a functional validation as the feature is central.

annetill · 2020-11-19T16:32:00Z

src/main/java/com/powsybl/openloadflow/network/impl/AbstractLfBus.java

-                            }
-                        });
-            }
+    public void setVoltageControl(boolean voltageControl) {


You have to renamed ths function for sure. I thought that you can make it using the Mode OFF of the voltageControl ?

No clear to me too what is the difference between mode of the VoltageControl and property voltageControlEnabled ? Is mode really usefull as mode is kind of result of individual voltage control enable status of all controllers?

I removed the mode, as VoltageControl is only structural, the state (voltageControlEnabled) remains in LfBus.

setVoltageControl() is moreover renamed into setVoltageControlEnabled()

annetill · 2020-11-19T16:32:53Z

src/main/java/com/powsybl/openloadflow/ac/equations/AcEquationSystem.java

-                if (!(creationParameters.isVoltageRemoteControl() && bus.getControlledBus().isPresent()) && bus.getControllerBuses().isEmpty()) {
-                    equationSystem.createEquation(bus.getNum(), EquationType.BUS_V).addTerm(new BusVoltageEquationTerm(bus, variableSet));
-                }
+            boolean hasLocalControl = bus.isVoltageController() && bus.getVoltageControl().isVoltageControlLocal();


This function could be more clear.

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

src/test/java/com/powsybl/openloadflow/ac/AcLoadFlowTransformerControlTest.java

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>

annetill · 2021-03-03T06:58:00Z

src/main/java/com/powsybl/openloadflow/network/impl/LfGeneratorImpl.java

+    @Override
+    public String getControlledBusId(boolean breakers) {
+        Terminal terminal = generator.getRegulatingTerminal();
+        Bus controlled = breakers ? terminal.getBusBreakerView().getBus() : terminal.getBusView().getBus();


You have to fix this: you have to check if the bus is not null and not creating the voltage control if it is. We do that for the discrete voltage control.

Done. In the last commit I do it earlier, in the LfGenerator constructor, so that the voltage control is disabled from the start.

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

annetill · 2021-03-03T07:47:08Z

src/main/java/com/powsybl/openloadflow/network/LfNetwork.java

-                controllerBusCount++;
-            }
-        }
+        long controllerBusCount = busesById.values().stream().filter(LfBus::isVoltageControllerEnabled).count();


This log is not equivalent to what was count before. Here we just want to know in all voltage control remote and local, how many controlled buses have at least one remote controller and for all controllers how many are remote. This is important if we want to compare.

Corrected, but what was counted before included the voltage controls discarded for too small max reactive ranges and for not started generators... so it's still annoying to compare the results.

annetill · 2021-03-03T08:25:42Z

src/main/java/com/powsybl/openloadflow/network/impl/LfNetworkLoaderImpl.java

@@ -68,7 +68,7 @@ private static void createVoltageControls(LfNetwork lfNetwork, List<LfBus> lfBus
                // check target voltage
                checkUniqueTargetVControllerBus(lfGenerator, controllerTargetV, controllerBus, generatorControlledBus);

-                if (lfGenerator.hasVoltageControl()) {
+                if (lfGenerator.hasVoltageControl() && controllerBus.isVoltageControllerEnabled()) {


This condition can be done before to avoid not usuful checks ?

Done; in fact some refactoring has been done here, to avoid the null/NaN initialization and the useless checks.

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

flo-dup · 2021-03-03T16:30:04Z

src/main/java/com/powsybl/openloadflow/network/impl/LfNetworkLoaderImpl.java

+                if (!voltageRemoteControl && controlledBus != controllerBus) {
+                    // if voltage remote control deactivated and remote control, set local control instead


This could be done earlier to have the lfGenerator consistent with the local voltage control, but for now it's easier to do it here.

geofjamg

Just minor remarks or questions. This is a large refactoring, lots of code has been moved and so this is very difficult to review. But new design looks good and is clearly better than previous one.

src/main/java/com/powsybl/openloadflow/network/LfNetwork.java

src/main/java/com/powsybl/openloadflow/network/VoltageControl.java

geofjamg · 2021-03-03T20:12:13Z

src/main/java/com/powsybl/openloadflow/network/VoltageControl.java

+        controllerBus.setVoltageControl(this);
+    }
+
+    public boolean isVoltageControlLocal() {


The name is not clear, at first read I was thinking it means this is only a local control

Indeed, but in fact we call it only once, to check just after if it is only local! hence I changed its content to return whether it is a local control only.

src/main/java/com/powsybl/openloadflow/network/impl/AbstractLfBus.java

geofjamg · 2021-03-03T20:17:47Z

src/test/java/com/powsybl/openloadflow/OpenLoadFlowParametersTest.java

@@ -164,7 +164,7 @@ void testMaxIterationReached() {
        parameters.setWriteSlackBus(true);
        Network network = EurostagTutorialExample1Factory.create();
        LoadFlow.Runner loadFlowRunner = new LoadFlow.Runner(new OpenLoadFlowProvider(new SparseMatrixFactory()));
-        network.getGenerator("GEN").setTargetV(5);
+        network.getVoltageLevelStream().forEach(vl -> vl.setNominalV(21));


That was just a way to get max iteration reached, as setting a target value too far from nominal voltage is not possible anymore. I improved it with a clearer and cleaner change, and added a comment.

geofjamg · 2021-03-03T20:18:37Z

src/test/java/com/powsybl/openloadflow/ac/GeneratorTargetVoltageInconsistencyTest.java

@@ -45,7 +45,7 @@ void localTest() {
                .setMinP(0)
                .setMaxP(200)
                .setTargetP(100)
-                .setTargetV(413)
+                .setTargetV(23)


Could you explain why you changed unit tests?

Because the previous test was not real with two generators connected to a 20 kV voltage level and with target V around 400 kV, without any remote regulating terminal.

geofjamg · 2021-03-03T20:22:26Z

src/main/java/com/powsybl/openloadflow/ac/equations/AcEquationSystemCreationParameters.java

    private final boolean phaseControl;

    private final boolean transformerVoltageControl;

    private final boolean forceA1Var;

-    public AcEquationSystemCreationParameters(boolean voltageRemoteControl, boolean phaseControl, boolean transformerVoltageControl) {
-        this(voltageRemoteControl, phaseControl, transformerVoltageControl, false);
+    public AcEquationSystemCreationParameters(boolean phaseControl, boolean transformerVoltageControl) {


Ok so remote voltage control de-activation is only managed at network creation.

Indeed, after that we do not need that boolean anymore.

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>

annetill · 2021-03-04T11:58:58Z

src/main/java/com/powsybl/openloadflow/network/impl/AbstractLfGenerator.java

+        // check that targetV has a plausible value (wrong nominal voltage issue)
+        if (targetV < PlausibleValues.MIN_TARGET_VOLTAGE_PU || targetV > PlausibleValues.MAX_TARGET_VOLTAGE_PU) {
+            LOGGER.warn("Generator '{}' has an inconsistent target voltage: {} pu", getId(), targetV);
+            newTargetV = (targetV > PlausibleValues.MAX_TARGET_VOLTAGE_PU) ? PlausibleValues.MAX_TARGET_VOLTAGE_PU : PlausibleValues.MIN_TARGET_VOLTAGE_PU;


@geofjamg Just tell if if this fix on targetV is okay for you.

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

sonarqubecloud · 2021-03-04T15:37:16Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

90.1% Coverage
0.0% Duplication

annetill

What a great work @floriand-e2r ! Thanks a lot !

flo-dup changed the title ~~Refactoring voltage control~~ [WIP] Refactoring voltage control Nov 10, 2020

flo-dup requested a review from annetill November 10, 2020 15:51

annetill requested changes Nov 10, 2020

View reviewed changes

annetill and others added 15 commits November 12, 2020 08:56

Add targetV for lfGenerator.

423642f

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>

Small rename.

3fe0c9b

First import of VoltageControl. Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>

Factorizing targetV in AbstractLfGenerator

4781e4a

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Set controller - controllee link thanks to lfGenerator

d9212f5

Remove the HashMap previously used Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Apply voltage control logic to LfDanglingLineBus

d667ad7

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Compute targetV after creating LfBuses

7a5766a

Done in createVoltageControls, together with various consistency checks Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Add link even if local voltage control

f49a85a

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Add local voltage control if voltageRemoteControl

2366b4c

AcEquationSystemCreationParameters.voltageRemoteControl now useless thus removed Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Small fixes.

67b765b

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>

All tests with same junit (#169)

d8626d8

Some tests were not included in build check as they were annotated with org.junit.api.Test Code smells corrections Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Change unit test to test max iteration reached.

0a981c9

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Refactor/correction if voltageRemoteControl disabled

6f0c3fb

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Use of VoltageControl parameter in AbstractLfBus

57e661d

Instead of controllersBus list / controlledBus optional Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Factorizing code adding generator to LfBus

d8b3498

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

flo-dup force-pushed the refactoring-voltage-control branch from d4f8013 to d8b3498 Compare November 17, 2020 14:41

flo-dup requested a review from annetill November 17, 2020 14:49

Merge branch 'master' into refactoring-voltage-control

931b438

annetill changed the title ~~[WIP] Refactoring voltage control~~ Refactoring voltage control Nov 19, 2020

annetill requested a review from geofjamg November 19, 2020 16:11

annetill requested changes Nov 19, 2020

View reviewed changes

jonenst force-pushed the master branch from 9542f6d to a371a95 Compare January 26, 2021 15:51

Merge branch 'master' into refactoring-voltage-control

0174be5

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

flo-dup commented Feb 9, 2021

View reviewed changes

src/test/java/com/powsybl/openloadflow/ac/AcLoadFlowTransformerControlTest.java Outdated Show resolved Hide resolved

flo-dup added 2 commits February 9, 2021 17:50

Correct transformer / generator voltage control conflict

95470d8

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Clean duplicate method

adb912b

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

annetill and others added 2 commits March 2, 2021 18:29

Merge branch 'master' into refactoring-voltage-control

17199f8

Fix merge.

5ac6420

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>

annetill reviewed Mar 3, 2021

View reviewed changes

flo-dup added 2 commits March 3, 2021 09:11

Fix: targetV retained only if voltage control on

eb8facb

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Fix: do not create voltage control if generator discarded

50ed801

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

annetill reviewed Mar 3, 2021

View reviewed changes

flo-dup added 6 commits March 3, 2021 09:27

Fix: log only remote controls

138143b

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Fix: check generators consistency only if LfGenerator voltage control on

2f08840

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

LfGenerator: check voltage control consistency in constructor

22f826f

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Fix: checking consistency of voltage control when setting it to a lfBus

8e51380

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Fix: check if the controlled bus is null

fadc481

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Fix: check if the regulating terminal is out of voltage in constructor

dc34b7c

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

flo-dup commented Mar 3, 2021

View reviewed changes

geofjamg requested changes Mar 3, 2021

View reviewed changes

Remove not useful else.

8f32bc4

Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>

annetill reviewed Mar 4, 2021

View reviewed changes

flo-dup added 8 commits March 4, 2021 13:03

Fix: iterate only once over buses in log size

3e52f18

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Add copyright

8ab915b

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Fix: isVoltageControlLocal returns whether control is only local

397e97e

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Fix: voltageControl parameter not stored as Optional<VoltageControl>

835334d

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Fix code smell

1c5866f

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Fix: test max iteration reached changed and commented

f29615c

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Fix: logs more info when target voltage rescaled

bfe1b31

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

Fix

8439788

Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>

geofjamg approved these changes Mar 4, 2021

View reviewed changes

annetill approved these changes Mar 4, 2021

View reviewed changes

flo-dup merged commit c3472c2 into master Mar 4, 2021

flo-dup deleted the refactoring-voltage-control branch March 4, 2021 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring voltage control #168

Refactoring voltage control #168

flo-dup commented Nov 10, 2020

annetill left a comment

sonarqubecloud bot commented Nov 19, 2020

annetill left a comment

annetill Nov 19, 2020

geofjamg Feb 15, 2021

flo-dup Feb 24, 2021

flo-dup Feb 24, 2021

annetill Nov 19, 2020

annetill Mar 3, 2021

flo-dup Mar 3, 2021

annetill Mar 3, 2021

flo-dup Mar 3, 2021

annetill Mar 3, 2021 •

edited

Loading

flo-dup Mar 3, 2021

flo-dup Mar 3, 2021

geofjamg left a comment

geofjamg Mar 3, 2021

flo-dup Mar 4, 2021

geofjamg Mar 3, 2021

flo-dup Mar 4, 2021

geofjamg Mar 3, 2021

annetill Mar 4, 2021

geofjamg Mar 3, 2021

flo-dup Mar 4, 2021

annetill Mar 4, 2021

geofjamg Mar 4, 2021

sonarqubecloud bot commented Mar 4, 2021

annetill left a comment

		if (!voltageRemoteControl && controlledBus != controllerBus) {
		// if voltage remote control deactivated and remote control, set local control instead

Refactoring voltage control #168

Refactoring voltage control #168

Conversation

flo-dup commented Nov 10, 2020

annetill left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 19, 2020

annetill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annetill Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geofjamg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 4, 2021

annetill left a comment

Choose a reason for hiding this comment

annetill Mar 3, 2021 •

edited

Loading