Maintain power factor constant on loads during slack distribution #170

frigaux · 2020-11-17T12:42:06Z

Power Factor Constant

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
#110 (comment)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature : add "power factor constant" support

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?
Improve equation resolution by Newton-Raphson with "powerFactorConstant: true" in config.yml (open-loadflow-default-parameters)

Does this PR introduce a breaking change or deprecate an API? If yes, check the following:

The Breaking Change or Deprecated label has been added
The migration guide has been updated in the github wiki (What changes might users need to make in their application due to this PR?)

Other information:

(if any of the questions/checkboxes don't apply, please delete them entirely)

annetill · 2020-11-17T13:33:23Z

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

+    @Test
+    void testPowerFactorConstant() {
+        // given
+        Network testNetwork = Importers.loadNetwork(Paths.get("src", "test", "resources", "2.xiidm"));


Use Network network = EurostagTutorialExample1Factory.create(); instead.

And thus delete the 2.xiidm file.

annetill · 2020-11-17T13:34:49Z

src/test/java/com/powsybl/openloadflow/util/LoadFlowAssert.java

@@ -47,4 +50,22 @@ public static void assertUndefinedActivePower(Terminal terminal) {
    public static void assertUndefinedReactivePower(Terminal terminal) {
        assertTrue(Double.isNaN(terminal.getQ()));
    }
+
+    public static void assertBetterResults(LoadFlowResult loadFlowResult, LoadFlowResult loadFlowResultBetter) {


Use assertLoadFlowResults instead of this name choice.

annetill · 2020-11-17T13:36:05Z

src/test/java/com/powsybl/openloadflow/util/LoadFlowAssert.java

+            assertEquals(componentResult.getSlackBusId(), componentResultBetter.getSlackBusId(), "this assert has a bug, please fix it");
+            assertEquals(componentResult.getStatus(), componentResultBetter.getStatus(), "status results should be the same");
+            assertEquals(componentResult.getIterationCount(), componentResultBetter.getIterationCount(), "iteration count results should be the same");
+            assertTrue(Math.abs(componentResult.getSlackBusActivePowerMismatch()) > Math.abs(componentResultBetter.getSlackBusActivePowerMismatch()), "it was expected that improved process returns a better result");


Why are you not doing here an assertEquals ?

It seems ending mismatch is smaller by making power factor a constant value.
Is it true ?
That's why my assert was called assertBetterResults.

I'm not sure this is true for all cases, but just a lucky side effect

annetill · 2020-11-17T13:39:10Z

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

+        LoadFlowResult loadFlowResult2 = loadFlowRunner.run(testNetwork, parameters);
+
+        // then
+        assertBetterResults(loadFlowResult1, loadFlowResult2);


I am not sure to understand why a result should be better than another one. Here the idea is to assert the reactive power that has changed in the second run. The assert on LoadFlowResults could be interested if you give reference loadflow results to avoid regression.

annetill · 2020-11-17T13:40:20Z

src/main/java/com/powsybl/openloadflow/util/ParameterConstants.java

@@ -28,6 +28,9 @@
    public static final String LOW_IMPEDANCE_BRANCH_MODE_PARAM_NAME = "lowImpedanceBranchMode";
    public static final LowImpedanceBranchMode LOW_IMPEDANCE_BRANCH_MODE_DEFAULT_VALUE = LowImpedanceBranchMode.REPLACE_BY_ZERO_IMPEDANCE_LINE;

+    public static final String POWER_FACTOR_CONSTANT_PARAM_NAME = "powerFactorConstant";


Do not forget to increase documentation. Our web site is here: https://github.com/powsybl/powsybl.github.io

Did you find the related page? it's that one: pages/documentation/simulation/powerflow/openlf.md

annetill · 2020-11-17T13:41:01Z

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

+    private LoadFlowParameters loadFlowParameters;
+    private OpenLoadFlowParameters openLoadFlowParameters;
+
+    public DistributedSlackOnLoadOuterLoop(LoadFlowParameters loadFlowParameters,


Can you here have as arguments only the parameters you need for this outerloop ?

From my mind, it's better to pass the two parameters objects instead of multiple primitive types ?
Moreover this code is done only in DistributedSlackOnLoadOuterLoop constructor (not in callers) :
this.distributedSlackOnConformLoad = loadFlowParameters.getBalanceType() == LoadFlowParameters.BalanceType.PROPORTIONAL_TO_CONFORM_LOAD;

So keep or not ?

I agree with Anne, here (as very often) it's better to have only the parameters you need in the constructor. That allows to see clearly which parameters affect the outerloop without even looking at the code in the outerloop. At the cost of more parameters for you to type, sure! Moreover, (very) minor thing, here you're testing twice the balance type.

mathbagu

Could you also please fix the DCO. You should configure your IDE to not forget it for your next commits. Please read the contributing guide if not already done.

mathbagu · 2020-11-17T15:12:10Z

src/main/java/com/powsybl/openloadflow/OpenLoadFlowParameters.java

+        return powerFactorConstant;
+    }
+
+    public void setPowerFactorConstant(boolean powerFactorConstant) {


We use a fluent API here: could you please follow the same way to implement setters?

mathbagu · 2020-11-17T15:14:25Z

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

-                            bus.getId(), targetP * PerUnit.SB, newTargetP * PerUnit.SB);
+                            bus.getId(), loadTargetP * PerUnit.SB, newLoadTargetP * PerUnit.SB);
+                }
+                if (this.openLoadFlowParameters.isPowerFactorConstant()) { // https://github.com/powsybl/powsybl-open-loadflow/issues/110


There is no need to prefix the openLoadFlowParaterers with this

mathbagu · 2020-11-17T15:18:22Z

src/test/java/com/powsybl/openloadflow/util/LoadFlowAssert.java

+            assertEquals(componentResult.getIterationCount(), componentResultBetter.getIterationCount(), "iteration count results should be the same");
+            assertTrue(Math.abs(componentResult.getSlackBusActivePowerMismatch()) > Math.abs(componentResultBetter.getSlackBusActivePowerMismatch()), "it was expected that improved process returns a better result");
+        }
+    }


As @annetill said, I'm not sure to understand this assessment. From my point of view, you should assert that the results are the expected one, not that the results are closed to the other one

…ent-726812696 Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

flo-dup

More general comments:

Please do not force-push after review, it makes it more difficult to see what recent changes you made. Which changes did you make with this morning's force push btw?
Please follow the guidelines for the commit message - or at least do not tick the box if you don't (I unticked it)!
In open-loadflow we require 90% coverage to merge a PR (the code coverage is above 92% so far in that repo), so please look at which conditions / lines are not covered with sonar and improve your tests to cover them

flo-dup · 2020-11-18T13:25:47Z

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

-    public DistributedSlackOnLoadOuterLoop(boolean throwsExceptionInCaseOfFailure, boolean distributedSlackOnConformLoad) {
-        super(throwsExceptionInCaseOfFailure);
-        this.distributedSlackOnConformLoad = distributedSlackOnConformLoad;
+    private LoadFlowParameters loadFlowParameters;


Field not used, remove it.

flo-dup · 2020-11-18T13:29:19Z

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

-        super(throwsExceptionInCaseOfFailure);
-        this.distributedSlackOnConformLoad = distributedSlackOnConformLoad;
+    private LoadFlowParameters loadFlowParameters;
+    private OpenLoadFlowParameters openLoadFlowParameters;


There's no use keeping the whole OpenLoadFlowParameters, remove it. Indeed, you only need to keep the value returned by isPowerFactorConstant().

flo-dup · 2020-11-18T13:44:07Z

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

+    private LoadFlowParameters loadFlowParameters;
+    private OpenLoadFlowParameters openLoadFlowParameters;
+
+    public DistributedSlackOnLoadOuterLoop(LoadFlowParameters loadFlowParameters,


I agree with Anne, here (as very often) it's better to have only the parameters you need in the constructor. That allows to see clearly which parameters affect the outerloop without even looking at the code in the outerloop. At the cost of more parameters for you to type, sure! Moreover, (very) minor thing, here you're testing twice the balance type.

flo-dup · 2020-11-18T13:52:51Z

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

+                    double loadTargetQ = bus.getLoadTargetQ();
+                    // keep power factor constant by using rule of three
+                    double newLoadTargetQ = loadTargetQ * newLoadTargetP / loadTargetP;
+                    if (LOGGER.isTraceEnabled()) {


Remove that if (and the existing similar if above), it's weighing down the code to only avoid the 2 multiplications by PerUnit.SB. Remember that the string concatenation is not done anyway if trace is not enabled.

flo-dup · 2020-11-18T14:02:26Z

src/main/java/com/powsybl/openloadflow/util/ParameterConstants.java

@@ -28,6 +28,9 @@
    public static final String LOW_IMPEDANCE_BRANCH_MODE_PARAM_NAME = "lowImpedanceBranchMode";
    public static final LowImpedanceBranchMode LOW_IMPEDANCE_BRANCH_MODE_DEFAULT_VALUE = LowImpedanceBranchMode.REPLACE_BY_ZERO_IMPEDANCE_LINE;

+    public static final String POWER_FACTOR_CONSTANT_PARAM_NAME = "powerFactorConstant";


Did you find the related page? it's that one: pages/documentation/simulation/powerflow/openlf.md

flo-dup · 2020-11-18T14:05:23Z

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

+    @Test
+    void testPowerFactorConstant() {
+        // given
+        Network testNetwork = Importers.loadNetwork(Paths.get("src", "test", "resources", "2.xiidm"));


And thus delete the 2.xiidm file.

…dFlowEngine, update Network Loads with new Q value in AbstractLfBus. Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

mathbagu · 2020-11-19T08:14:25Z

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

    private boolean distributedSlackOnConformLoad = false;
+    private boolean powerFactorConstant = false;


These two fields should be final?

mathbagu · 2020-11-19T08:17:39Z

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

+            if (newLoadTargetP != loadTargetP) {
+                LOGGER.trace("Rescale '{}' active power target: {} -> {}",
+                        bus.getId(), loadTargetP * PerUnit.SB, newLoadTargetP * PerUnit.SB);
+                if (powerFactorConstant) { // https://github.com/powsybl/powsybl-open-loadflow/issues/110


I think a sentence to explain the purpose of this block of code would be more useful and convenient than a link to the issue lists.

mathbagu · 2020-11-19T08:19:08Z

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

+                    .setP(load.getP0() >= 0 ? factorP * load.getP0() : load.getP0())
+                    .setQ(load.getQ0() >= 0 ? factorQ * load.getQ0() : load.getQ0());


@annetill: why do we distinguish positive and negative loads?

I think these tests are not useful, I have to check.

Mathieu request changes (PR 170) : Resolved request changes Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

…powerFactorConstant = true» in this case, we have to remain power factor constant on variable power for P and Q Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

… mvn test) : cannot do assertEquals with full precision on slackBusActivePowerMismatch, between Java 8 to Java 11, it seems double primitive type has some differences between Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

use delta from assert instead of Math.round Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

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

Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

flo-dup · 2020-11-24T14:17:02Z

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

    }

    private double getVariableLoadTargetP(LfBus bus) {
        return distributedSlackOnConformLoad ? bus.getLoadTargetP() - bus.getFixedLoadTargetP() : bus.getLoadTargetP();
    }

+    private double getVariableLoadTargetQAgainstVariableLoadTargetP(LfBus bus) {
+        return distributedSlackOnConformLoad ? (bus.getLoadTargetQ() - bus.getFixedLoadTargetQ()) / (bus.getLoadTargetP() - bus.getFixedLoadTargetP())


Maybe check that bus.getLoadTargetP() != bus.getFixedLoadTargetP() to avoid the division by 0.

Don't know why, but it seems we cannot get division by 0 in reality (tested with recollement-auto-20190124-2110-enrichi.xiidm) ?!?

However we can fix this in getVariableLoadTargetQAgainstVariableLoadTargetP with :

throwing a ComputationException ?

or returning 0 ?

Indeed you check before that the variable part is greater than zero to participate.

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

…wsybl-open-loadflow into power_factor_constant

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

# Conflicts: # src/main/java/com/powsybl/openloadflow/OpenLoadFlowProvider.java

Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@rte-france.com>

geofjamg

As loadTargetQ might be modified, it has to be taken into account in security analysis state restoration (between 2 contingencies)

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

…ty from 16 to the 15 allowed." Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

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

…wsybl-open-loadflow into power_factor_constant

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

sonarqubecloud · 2020-12-01T14:26:22Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

90.0% Coverage
0.0% Duplication

frigaux changed the title ~~https://github.com/powsybl/powsybl-open-loadflow/issues/110#issuecomm…~~ Power Factor Constant Nov 17, 2020

annetill requested review from annetill and flo-dup November 17, 2020 13:27

annetill requested changes Nov 17, 2020

View reviewed changes

mathbagu requested changes Nov 17, 2020

View reviewed changes

https://github.com/powsybl/powsybl-open-loadflow/issues/110#issuecomm…

f6b82b5

…ent-726812696 Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

frigaux force-pushed the power_factor_constant branch from fe68636 to f6b82b5 Compare November 18, 2020 09:06

flo-dup requested changes Nov 18, 2020

View reviewed changes

Updates and fixes about Pull Request 170. Once run completed in Acloa…

757364e

…dFlowEngine, update Network Loads with new Q value in AbstractLfBus. Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

mathbagu reviewed Nov 19, 2020

View reviewed changes

frigaux and others added 6 commits November 19, 2020 10:57

Checkstyle : fixed checkstyle errors

1b500df

Mathieu request changes (PR 170) : Resolved request changes Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

fixed checkstyle indentation error when running mvn validate

06b19e9

Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

implement case : « BalanceType = PROPORTIONAL_TO_CONFORM_LOAD » et « …

97e52bb

…powerFactorConstant = true» in this case, we have to remain power factor constant on variable power for P and Q Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

code style

f36e241

use delta from assert instead of Math.round Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

Some simplifications and clarifications.

b9cd0bc

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

annetill requested a review from flo-dup November 24, 2020 12:06

annetill and others added 2 commits November 24, 2020 14:28

Rename method after review.

9abfe0c

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

Last misses : comment and log

ce468d1

Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

flo-dup requested changes Nov 24, 2020

View reviewed changes

annetill added 2 commits November 24, 2020 17:05

Fix.

2b890d6

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

Merge branch 'power_factor_constant' of https://github.com/powsybl/po…

5f6d9a4

…wsybl-open-loadflow into power_factor_constant

annetill changed the title ~~Power Factor Constant~~ Maintain power factor constant on loads during slack distribution Nov 24, 2020

annetill and others added 5 commits November 24, 2020 17:24

Add if for trace.

e9cf603

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

Fix.

64fbdd1

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

Remove isTraceEnabled at some places.

d0708d5

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

Merge branch 'master' into power_factor_constant

9dd5da3

# Conflicts: # src/main/java/com/powsybl/openloadflow/OpenLoadFlowProvider.java

Fix authorship and copyright

88371cd

Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@rte-france.com>

geofjamg requested changes Nov 26, 2020

View reviewed changes

Merge branch 'master' into power_factor_constant

5bebe97

annetill and others added 8 commits November 27, 2020 18:00

Fixes after review.

c95d352

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

fix code smell "Refactor this method to reduce its Cognitive Complexi…

e9aedff

…ty from 16 to the 15 allowed." Signed-off-by: Fabien Rigaux <fabien.rigaux@free.fr>

Fix.

671e350

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

Power factor is on global P0 and Q0.

8b25121

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

Merge branch 'master' into power_factor_constant

8f16cc5

Merge branch 'power_factor_constant' of https://github.com/powsybl/po…

a24750b

…wsybl-open-loadflow into power_factor_constant

Simplification consecutive to previous commit (power factor on P0, Q0)

ff60ee3

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

Renaming: "constant" instead of "remains constant"

d07e80b

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

flo-dup approved these changes Dec 1, 2020

View reviewed changes

annetill merged commit a66ea7d into master Dec 1, 2020

annetill deleted the power_factor_constant branch December 1, 2020 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintain power factor constant on loads during slack distribution #170

Maintain power factor constant on loads during slack distribution #170

frigaux commented Nov 17, 2020 •

edited by flo-dup

Loading

annetill Nov 17, 2020

flo-dup Nov 18, 2020

annetill Nov 17, 2020

annetill Nov 17, 2020

frigaux Nov 17, 2020

mathbagu Nov 19, 2020

annetill Nov 17, 2020

annetill Nov 17, 2020

flo-dup Nov 18, 2020

annetill Nov 17, 2020

frigaux Nov 17, 2020

flo-dup Nov 18, 2020

mathbagu left a comment

mathbagu Nov 17, 2020

mathbagu Nov 17, 2020

mathbagu Nov 17, 2020

flo-dup left a comment

flo-dup Nov 18, 2020

flo-dup Nov 18, 2020

flo-dup Nov 18, 2020

flo-dup Nov 18, 2020

flo-dup Nov 18, 2020

flo-dup Nov 18, 2020

mathbagu Nov 19, 2020

mathbagu Nov 19, 2020

mathbagu Nov 19, 2020

annetill Nov 24, 2020

flo-dup Nov 24, 2020

frigaux Nov 24, 2020

annetill Nov 24, 2020

geofjamg left a comment

sonarqubecloud bot commented Dec 1, 2020

		private boolean distributedSlackOnConformLoad = false;
		private boolean powerFactorConstant = false;

		.setP(load.getP0() >= 0 ? factorP * load.getP0() : load.getP0())
		.setQ(load.getQ0() >= 0 ? factorQ * load.getQ0() : load.getQ0());

Maintain power factor constant on loads during slack distribution #170

Maintain power factor constant on loads during slack distribution #170

Conversation

frigaux commented Nov 17, 2020 • edited by flo-dup Loading

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

Choose a reason for hiding this comment

mathbagu 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

flo-dup 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

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

sonarqubecloud bot commented Dec 1, 2020

frigaux commented Nov 17, 2020 •

edited by flo-dup

Loading