Share code for network modifications #524

Mathieu-Deharbe · 2024-08-28T08:44:49Z

No description provided.

Signed-off-by: Mathieu DEHARBE <[email protected]>

…ions' into share-code-for-network-modifications

Signed-off-by: Mathieu DEHARBE <[email protected]>

ghazwarhili · 2024-09-12T20:30:12Z

src/main/java/org/gridsuite/modification/server/dto/formula/equipmentfield/BatteryField.java

@@ -36,14 +43,28 @@ public static Double getReferenceValue(Battery battery, String batteryField) {

    public static void setNewValue(Battery battery, String batteryField, Double newValue) {
        BatteryField field = BatteryField.valueOf(batteryField);
+        final AttributeModification<Double> modif = new AttributeModification<>(newValue, OperationType.SET);


doubleAttributeModification ? or attributeModification
use variable name more descriptive

Is attributeModif enough ?

Ok I renamed it to the whole attributeModification.

ghazwarhili · 2024-09-12T20:30:37Z

src/main/java/org/gridsuite/modification/server/dto/formula/equipmentfield/BatteryField.java

+            case ACTIVE_POWER_SET_POINT -> {
+                ModificationUtils.getInstance().checkActivePowerZeroOrBetweenMinAndMaxActivePower(
+                        modif, null, null, battery.getMinP(),
+                        battery.getMaxP(), battery.getTargetP(), MODIFY_GENERATOR_ERROR, "Battery '" + battery.getId() + "' : "


MODIFY_BATTERY_ERROR

ghazwarhili · 2024-09-12T20:46:08Z

src/main/java/org/gridsuite/modification/server/dto/formula/equipmentfield/BatteryField.java

+                ActivePowerControlAdder<Battery> activePowerControlAdder = battery.newExtension(ActivePowerControlAdder.class);
+                ModificationUtils.getInstance().modifyActivePowerControlAttributes(
+                        activePowerControl, activePowerControlAdder, null,
+                        new AttributeModification<>(newValue.floatValue(), OperationType.SET), null, null);


check for null before converting it to a float
you can use optional to handle null safely like
Optional.ofNullable(newValue) .map(Double::floatValue) ...

I added @NotNull to the Double newValue parameter.

ghazwarhili · 2024-09-12T20:46:57Z

src/main/java/org/gridsuite/modification/server/dto/formula/equipmentfield/BatteryField.java

+import org.gridsuite.modification.server.dto.OperationType;
+import org.gridsuite.modification.server.modifications.ModificationUtils;
+
+import static org.gridsuite.modification.server.NetworkModificationException.Type.MODIFY_GENERATOR_ERROR;


Battery exception type !

ghazwarhili · 2024-09-12T20:47:33Z

src/main/java/org/gridsuite/modification/server/dto/formula/equipmentfield/GeneratorField.java

            GeneratorField field = GeneratorField.valueOf(generatorField);
+            final AttributeModification<Double> attrModif = new AttributeModification<>(newValue, OperationType.SET);


And same as well.

ghazwarhili · 2024-09-12T21:04:03Z

...rg/gridsuite/modification/server/dto/formula/equipmentfield/TwoWindingsTransformerField.java

-            case PHASE_LOW_TAP_POSITION -> phaseTapChanger.setLowTapPosition(newValue.intValue());
-            case PHASE_TAP_POSITION -> phaseTapChanger.setTapPosition(newValue.intValue());
-            case PHASE_TARGET_DEADBAND -> phaseTapChanger.setTargetDeadband(newValue);
+            case R -> modifyBranchFields(transformer, attrModif, null, null);


modifyAttributeR ? or modifyR ?
same remarq for X attribute

I am not sure why but those field got put together and called branch specific fields in the function modifyCharacteristics. They also have a BranchModificationInfos object joining them. So I kept them together.

But ok let's separate them. We'll see.

ghazwarhili · 2024-09-12T21:12:36Z

src/main/java/org/gridsuite/modification/server/modifications/ModificationUtils.java

                                                         ReportNode subReporterSetpoints) {
        List<ReportNode> reports = new ArrayList<>();
        if (activePowerControl != null) {
            modifyExistingActivePowerControl(activePowerControl, participateInfo, droopInfo, reports);
        } else {
            createNewActivePowerControl(activePowerControlAdder, participateInfo, droopInfo, reports);
        }
+        if (subReportNode == null) {


Because the end of the function is all about reports. But this is not clear, I reversed it. Let me know if this is ok.

ghazwarhili · 2024-09-12T21:16:31Z

src/main/java/org/gridsuite/modification/server/modifications/VoltageLevelModification.java

+                                                      AttributeModification<Double> ipMax,
+                                                      ReportNode subReportNode,
+                                                      VoltageLevel voltageLevel) {
+        if (ipMin == null && ipMax == null) {


we have to report this case ?

I don't think so : in the case of a regular network modification most value are null (which means not modified). So this case is simply ignored. In the case of modification by formula, null fields are detected much sooner.

ghazwarhili · 2024-09-13T06:36:05Z

src/main/java/org/gridsuite/modification/server/modifications/VoltageLevelModification.java

+                    .buildModificationReport(oldIpMaxToReport, newIpMaxToReport, "High short circuit current limit"));
+        } else if (oldIpMax != null) {
+            identifiableShortCircuitAdder.withIpMax(oldIpMax);
+        }


to optimize and refacto this code I suggest something like

private void updateShortCircuitLimits(ValueWrapper ipMin, ValueWrapper ipMax, Double oldIpMin, Double oldIpMax, IdentifiableShortCircuitAdder identifiableShortCircuitAdder, List<ModificationReport> reports) { updateShortCircuitLimit(ipMin, oldIpMin, "Low short circuit current limit", identifiableShortCircuitAdder::withIpMin, reports); updateShortCircuitLimit(ipMax, oldIpMax, "High short circuit current limit", identifiableShortCircuitAdder::withIpMax, reports); } private void updateShortCircuitLimit(ValueWrapper newValueWrapper, Double oldValue, String limitDescription, Consumer<Double> setterMethod, List<ModificationReport> reports) { Double newValue = (newValueWrapper != null) ? newValueWrapper.getValue() : null; if (newValue != null) { setterMethod.accept(newValue); Double oldValueToReport = convertToKiloAmps(oldValue); Double newValueToReport = convertToKiloAmps(newValue); reports.add(ModificationUtils.getInstance().buildModificationReport(oldValueToReport, newValueToReport, limitDescription)); } else if (oldValue != null) { setterMethod.accept(oldValue); } } private Double convertToKiloAmps(Double value) { return (value != null) ? value * 0.001 : null; }

Looks nice. I tried to integrate it but it broke the unit tests, I don't know why. So I just kept convertToKiloAmps.

ghazwarhili · 2024-09-13T06:43:02Z

...java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerModification.java

+                double oldMagnetizingConductanceToReport = transformer.getG() * Math.pow(10, 6);
+                double newMagnetizingConductanceToReport = modifG.getValue() * Math.pow(10, 6);
+                insertReportNode(reportNode, ModificationUtils.getInstance().buildModificationReportWithIndentation(oldMagnetizingConductanceToReport,
+                        newMagnetizingConductanceToReport, "Magnetizing conductance", 1));


Use Constants for Keys

Like this ?

EtienneLt

little comments

EtienneLt · 2024-09-17T08:22:56Z

...java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerModification.java

-            twoWindingsTransformer.setRatedU1(twoWindingsTransformerModificationInfos.getRatedU1().getValue());
+    }
+
+    public static void modifyBranchFields(TwoWindingsTransformer twt, AttributeModification<Double> modifR, AttributeModification<Double> modifX, ReportNode reportNode) {


change name and maybe separate in two methods

one for R and one for X

Haha you agree with Ghazwa. Done.

...java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerModification.java

...java/org/gridsuite/modification/server/modifications/GeneratorByFormulaModificationTest.java

Signed-off-by: Mathieu DEHARBE <[email protected]>

…ion-server into share-code-for-network-modifications Signed-off-by: Mathieu DEHARBE <[email protected]> # Conflicts: # src/main/java/org/gridsuite/modification/server/modifications/GeneratorModification.java # src/main/java/org/gridsuite/modification/server/modifications/ModificationUtils.java # src/main/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerModification.java

Signed-off-by: Mathieu DEHARBE <[email protected]>

…ions' into share-code-for-network-modifications

Signed-off-by: Mathieu DEHARBE <[email protected]>

Mathieu-Deharbe

Ok I renamed it to the whole attributeModification.

Many merge conflicts are solved in the last commits.

Mathieu-Deharbe · 2024-09-17T14:03:29Z

src/main/java/org/gridsuite/modification/server/dto/formula/equipmentfield/BatteryField.java

@@ -36,14 +43,28 @@ public static Double getReferenceValue(Battery battery, String batteryField) {

    public static void setNewValue(Battery battery, String batteryField, Double newValue) {
        BatteryField field = BatteryField.valueOf(batteryField);
+        final AttributeModification<Double> modif = new AttributeModification<>(newValue, OperationType.SET);


Is attributeModif enough ?

Mathieu-Deharbe · 2024-09-17T14:05:09Z

src/main/java/org/gridsuite/modification/server/dto/formula/equipmentfield/BatteryField.java

+                ActivePowerControlAdder<Battery> activePowerControlAdder = battery.newExtension(ActivePowerControlAdder.class);
+                ModificationUtils.getInstance().modifyActivePowerControlAttributes(
+                        activePowerControl, activePowerControlAdder, null,
+                        new AttributeModification<>(newValue.floatValue(), OperationType.SET), null, null);


I added @NotNull to the Double newValue parameter.

Mathieu-Deharbe · 2024-09-17T14:05:49Z

src/main/java/org/gridsuite/modification/server/dto/formula/equipmentfield/GeneratorField.java

            GeneratorField field = GeneratorField.valueOf(generatorField);
+            final AttributeModification<Double> attrModif = new AttributeModification<>(newValue, OperationType.SET);


And same as well.

Mathieu-Deharbe · 2024-09-17T14:06:59Z

src/main/java/org/gridsuite/modification/server/dto/formula/equipmentfield/LoadField.java

@@ -28,8 +33,8 @@ public static Double getReferenceValue(Load load, String loadField) {
    public static void setNewValue(Load load, String loadField, Double newValue) {
        LoadField field = LoadField.valueOf(loadField);
        switch (field) {
-            case ACTIVE_POWER -> load.setP0(newValue);
-            case REACTIVE_POWER -> load.setQ0(newValue);
+            case ACTIVE_POWER -> modifyP0(load, new AttributeModification<>(newValue, OperationType.SET), null);


But P0 is an attribute. Therefore attribute is implicit ?

Mathieu-Deharbe · 2024-09-17T14:07:10Z

src/main/java/org/gridsuite/modification/server/dto/formula/equipmentfield/LoadField.java

-            case ACTIVE_POWER -> load.setP0(newValue);
-            case REACTIVE_POWER -> load.setQ0(newValue);
+            case ACTIVE_POWER -> modifyP0(load, new AttributeModification<>(newValue, OperationType.SET), null);
+            case REACTIVE_POWER -> modifyQ0(load, new AttributeModification<>(newValue, OperationType.SET), null);


Same remark.

Mathieu-Deharbe · 2024-09-17T15:16:47Z

...java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerModification.java

-            twoWindingsTransformer.setRatedU1(twoWindingsTransformerModificationInfos.getRatedU1().getValue());
+    }
+
+    public static void modifyBranchFields(TwoWindingsTransformer twt, AttributeModification<Double> modifR, AttributeModification<Double> modifX, ReportNode reportNode) {


Haha you agree with Ghazwa. Done.

...java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerModification.java

Mathieu-Deharbe · 2024-09-17T15:34:32Z

src/main/java/org/gridsuite/modification/server/modifications/VoltageLevelModification.java

+                                                      AttributeModification<Double> ipMax,
+                                                      ReportNode subReportNode,
+                                                      VoltageLevel voltageLevel) {
+        if (ipMin == null && ipMax == null) {


I don't think so : in the case of a regular network modification most value are null (which means not modified). So this case is simply ignored. In the case of modification by formula, null fields are detected much sooner.

Mathieu-Deharbe · 2024-09-17T16:15:54Z

src/main/java/org/gridsuite/modification/server/modifications/VoltageLevelModification.java

+                    .buildModificationReport(oldIpMaxToReport, newIpMaxToReport, "High short circuit current limit"));
+        } else if (oldIpMax != null) {
+            identifiableShortCircuitAdder.withIpMax(oldIpMax);
+        }


Looks nice. I tried to integrate it but it broke the unit tests, I don't know why. So I just kept convertToKiloAmps.

...java/org/gridsuite/modification/server/modifications/GeneratorByFormulaModificationTest.java

EtienneLt

A few more comments and it is good for me

EtienneLt · 2024-09-24T08:51:53Z

src/main/java/org/gridsuite/modification/server/dto/formula/equipmentfield/GeneratorField.java

@@ -61,86 +66,42 @@ public static Double getReferenceValue(Generator generator, String generatorFiel
        };
    }

-    public static void setNewValue(Generator generator, String generatorField, Double newValue) {
+    public static void setNewValue(Generator generator, String generatorField, @NotNull Double newValue) {


should it be better to set a primitive type (double), it forces to be not null

Why not. Done.

EtienneLt · 2024-09-24T08:55:46Z

src/main/java/org/gridsuite/modification/server/modifications/ByFormulaModification.java

                }
            }
+            report(formulaSubReporter, formulaReports);


are you sure ? the second

report(formulaSubReporter, formulaReports);

is in another if loop

Yes, pretty sure.
Previously if none of your equipment had been modified those reports were not displayed even if it was because of a problem in the network or the formula.
I think it is much better to have the details. This way the user can display the TRACE in the logs if he wants to, and see what really happened.

EtienneLt · 2024-09-24T09:15:44Z

src/main/java/org/gridsuite/modification/server/modifications/GeneratorModification.java

+        ReportNode reportReactivePower = null;
+        if (modifTargetQ != null) {
+            if (modifTargetQ.getOp() == OperationType.SET) {
+                reportReactivePower = ModificationUtils.getInstance().applyElementaryModificationsAndReturnReport(generator::setTargetQ, generator::getTargetQ, modifTargetQ, "Reactive power");


should it be specified that it is target reactive power or setpoint ?

I was simply refactoring that part but I think that you are right. Ok renamed.

Signed-off-by: Mathieu DEHARBE <[email protected]>

sonarqubecloud · 2024-09-25T10:43:50Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
92.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

…s by formula (#524) Signed-off-by: Mathieu DEHARBE <[email protected]>

Mathieu-Deharbe and others added 17 commits August 28, 2024 10:44

draft

ad6f6f3

Signed-off-by: Mathieu DEHARBE <[email protected]>

draft refacto check and apply functions

3b27dfb

Signed-off-by: Mathieu DEHARBE <[email protected]>

indent and correct TU

1986a26

Signed-off-by: Mathieu DEHARBE <[email protected]>

finish GeneratorField + correction in GeneratorModification

dffe039

Signed-off-by: Mathieu DEHARBE <[email protected]>

shunt compensator MAXIMUM_SECTION_COUNT

3c1c0fa

Signed-off-by: Mathieu DEHARBE <[email protected]>

Merge branch 'main' into share-code-for-network-modifications

21cc44e

shunt compensator

f4a62eb

Signed-off-by: Mathieu DEHARBE <[email protected]>

Merge remote-tracking branch 'origin/share-code-for-network-modificat…

bccd1cb

…ions' into share-code-for-network-modifications

VoltageLevelFields

52a7eeb

Signed-off-by: Mathieu DEHARBE <[email protected]>

reduce modifyVoltageLevelShortCircuit code complexity

4dcf358

Signed-off-by: Mathieu DEHARBE <[email protected]>

always send formulaReports

2ce33ed

Signed-off-by: Mathieu DEHARBE <[email protected]>

refacto BatteryField

c3bbff8

Signed-off-by: Mathieu DEHARBE <[email protected]>

refacto GeneratorField

fe01d83

Signed-off-by: Mathieu DEHARBE <[email protected]>

refacto VoltageLevelField

3337dd9

Signed-off-by: Mathieu DEHARBE <[email protected]>

correct tests

c37e5a0

Signed-off-by: Mathieu DEHARBE <[email protected]>

correct tests

a8ae013

Signed-off-by: Mathieu DEHARBE <[email protected]>

modifyBranchValues

906613b

Signed-off-by: Mathieu DEHARBE <[email protected]>

Mathieu-Deharbe requested review from basseche and EtienneLt September 4, 2024 11:40

Mathieu-Deharbe added 4 commits September 4, 2024 14:20

finish TwoWindingsTransformerField

6f7a6ae

Signed-off-by: Mathieu DEHARBE <[email protected]>

reduce cognitive complexity

6e29fdc

Signed-off-by: Mathieu DEHARBE <[email protected]>

small refect on BatteryField

d249d45

Signed-off-by: Mathieu DEHARBE <[email protected]>

ultimate cleaning

90b6b5c

Signed-off-by: Mathieu DEHARBE <[email protected]>

ghazwarhili self-requested a review September 12, 2024 09:54

ghazwarhili reviewed Sep 13, 2024

View reviewed changes

EtienneLt requested changes Sep 17, 2024

View reviewed changes

Mathieu-Deharbe and others added 4 commits September 17, 2024 18:16

corections post review

e758123

Signed-off-by: Mathieu DEHARBE <[email protected]>

Merge branch 'main' into share-code-for-network-modifications

5c7f145

revert to correct unit tests

68e970a

Signed-off-by: Mathieu DEHARBE <[email protected]>

Mathieu-Deharbe added 2 commits September 18, 2024 12:32

Merge remote-tracking branch 'origin/share-code-for-network-modificat…

04495ce

…ions' into share-code-for-network-modifications

clean imports

55de855

Signed-off-by: Mathieu DEHARBE <[email protected]>

Mathieu-Deharbe commented Sep 18, 2024

View reviewed changes

Mathieu-Deharbe requested review from ghazwarhili and EtienneLt September 18, 2024 10:52

Merge branch 'main' into share-code-for-network-modifications

c9ce267

basseche approved these changes Sep 19, 2024

View reviewed changes

Merge branch 'main' into share-code-for-network-modifications

6b35eec

EtienneLt reviewed Sep 24, 2024

View reviewed changes

post review

1bf83ed

Signed-off-by: Mathieu DEHARBE <[email protected]>

Mathieu-Deharbe requested a review from EtienneLt September 25, 2024 10:45

EtienneLt approved these changes Sep 25, 2024

View reviewed changes

Mathieu-Deharbe merged commit bc45748 into main Sep 26, 2024
3 checks passed

Mathieu-Deharbe deleted the share-code-for-network-modifications branch September 26, 2024 08:07

Mathieu-Deharbe mentioned this pull request Sep 26, 2024

adjusting regulating when modifying phase tap changer #537

Merged

EtienneLt pushed a commit that referenced this pull request Oct 8, 2024

share code for regular network modifications and network modification…

21e8e08

…s by formula (#524) Signed-off-by: Mathieu DEHARBE <[email protected]>

		GeneratorField field = GeneratorField.valueOf(generatorField);
		final AttributeModification<Double> attrModif = new AttributeModification<>(newValue, OperationType.SET);

Share code for network modifications #524

Share code for network modifications #524

Conversation

Mathieu-Deharbe commented Aug 28, 2024

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

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

EtienneLt 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

Mathieu-Deharbe 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

Mathieu-Deharbe Sep 17, 2024 • edited 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

EtienneLt left a comment • edited 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

sonarqubecloud bot commented Sep 25, 2024

Quality Gate passed

Mathieu-Deharbe Sep 17, 2024 •

edited

Loading

EtienneLt left a comment •

edited

Loading