Refactor bus models #261

Lisrte · 2023-07-03T13:11:25Z

Please check if the PR fulfills these requirements

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)

What kind of change does this PR introduce?

Bug fix

What is the new behavior (if this is a feature change)?
Split Bus model in three based on use case :

connection point for equipment connection (use static id for macro connect with default bus)
measurement point for automaton (replace default bus model)
bus of equipment for omegaref (use equipment id instead of bus id for macro connect)

Does this PR introduce a breaking change or deprecate an API?

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?)

- connection point for equipment connection (use static id for macro connect with default bus) - measurement point for automaton (replace default bus model) - bus of equipment for omegaref (use equipment id instead of bus id for macro connect) Signed-off-by: lisrte <[email protected]>

Signed-off-by: lisrte <[email protected]>

gautierbureau · 2023-07-04T15:28:22Z

I need to look a bit more into this but I like the Connection/Measurement concepts

dynawaltz/src/main/java/com/powsybl/dynawaltz/models/buses/StaticDefaultBusModel.java

Signed-off-by: lisrte <[email protected]>

sonarqubecloud · 2023-07-07T15:12:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

91.4% Coverage
0.0% Duplication

Signed-off-by: lisrte <[email protected]>

Use 'eager initialized singleton' implementation for DefaultBus Signed-off-by: lisrte <[email protected]>

flo-dup

Nice refactor

dynawaltz/src/main/java/com/powsybl/dynawaltz/models/buses/DefaultBusOfEquipment.java

dynawaltz/src/main/java/com/powsybl/dynawaltz/models/buses/MeasurementPoint.java

dynawaltz/src/main/java/com/powsybl/dynawaltz/models/buses/ConnectionPoint.java

Signed-off-by: lisrte <[email protected]>

Signed-off-by: Florian Dupuy <[email protected]>

flo-dup

Sorry, some more comments/questions to discuss!

flo-dup · 2023-10-13T13:45:51Z

dynawaltz/src/main/java/com/powsybl/dynawaltz/models/buses/DefaultBus.java

-        super(staticId);
+public final class DefaultBus implements EquipmentConnectionPoint {
+
+    private static final DefaultBus INSTANCE = new DefaultBus();


I'm wondering if we really should use the singleton pattern here. You're using it as an optimization, but I think the memory waste doesn't matter that much regarding the loss of readability and logic. There is one class for all the buses, which might be a bit confusing. It doesn't inherit AbstractDefaultModel, which is also unexpected. And it isn't handled with DefaultModelsHandler, but that's the limit of this PR, as you may have 3 models for the same bus.

flo-dup · 2023-10-13T14:32:14Z

dynawaltz/src/main/java/com/powsybl/dynawaltz/models/buses/AbstractBus.java

@@ -20,7 +20,7 @@
 /**
 * @author Laurent Issertial <laurent.issertial at rte-france.com>
 */
-public abstract class AbstractBus extends AbstractEquipmentBlackBoxModel<Bus> implements BusModel {
+public abstract class AbstractBus extends AbstractEquipmentBlackBoxModel<Bus> implements EquipmentConnectionPoint, ActionConnectionPoint {


I think you can remove ActionConnectionPoint here, it's not needed anymore

flo-dup · 2023-10-13T14:33:43Z

dynawaltz/src/main/java/com/powsybl/dynawaltz/models/buses/ActionConnectionPoint.java


-    Optional<String> getNumCCVarName();
+    String getTerminalVarName();

    Optional<String> getUImpinVarName();


And then it could be String getUImpinVarName(); and String getUpuImpinVarName(); instead of the optional

flo-dup · 2023-10-13T14:50:29Z

dynawaltz/src/main/java/com/powsybl/dynawaltz/models/frequencysynchronizers/OmegaRef.java

    }

    @Override
    public void createMacroConnections(DynaWaltzContext context) throws PowsyblException {
        int index = 0;
        for (FrequencySynchronizedModel eq : synchronizedEquipments) {
            createMacroConnections(eq, getVarConnectionsWith(eq), context, MacroConnectAttribute.ofIndex1(index));
-            createMacroConnections(eq.getConnectedBusId(), BusModel.class, this::getVarConnectionsWith, context, MacroConnectAttribute.ofIndex1(index));
+            BusOfFrequencySynchronizedModel busOf = new DefaultBusOfFrequencySynchronized(eq.getStaticId());


You're using a default ("network") model here even if there is a specific bus model on the corresponding bs, I'm not sure that's ok for dynawo, could you confirm that @gautierbureau?

So in the dyd we might have

<dyn:macroConnector id="MC1"> <dyn:connect var1="numcc_node_@INDEX@" var2="@@NAME@@@NODE@_numcc"/> </dyn:macroConnector> <dyn:macroConnect connector="MC1" id1="FREQ_SYNC" index1="0" id2="NETWORK" name2="GEN"/>

And the node mentioned in var2 would be a bus which has got a StandardBus or InfiniteBus model mapped on it.

We need to have a default bus model other types of models would not work with OmegaRef

flo-dup · 2023-10-13T15:06:17Z

dynawaltz/src/main/java/com/powsybl/dynawaltz/models/defaultmodels/DefaultModelsHandler.java

@@ -53,7 +53,7 @@ public DefaultModelsHandler() {
                StaticVarCompensatorModel.class, new DefaultModelFactory<StaticVarCompensatorModel>(DefaultStaticVarCompensator::new),
                TransformerModel.class, new DefaultModelFactory<TransformerModel>(DefaultTransformer::new));

-        powSyBlTypeToModel.put(IdentifiableType.BUS, BusModel.class);
+        powSyBlTypeToModel.put(IdentifiableType.BUS, ActionConnectionPoint.class);


So a bus is always a ActionConnectionPoint once you're here, you were forced to bypass this handler for the two other bus interfaces. Ok about that, at least to move on, but could you put a comment here mentioning the two others default bus models being bypassed?

Signed-off-by: lisrte <[email protected]>

…nt getName method Signed-off-by: lisrte <[email protected]>

sonarqubecloud · 2023-10-18T09:22:11Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

91.1% Coverage
0.0% Duplication

Lisrte added 2 commits July 3, 2023 11:31

Replace OmegaRef with SetPoint when a StandardBus is found

a0c3154

Signed-off-by: lisrte <[email protected]>

Lisrte requested a review from gautierbureau July 3, 2023 13:11

gautierbureau reviewed Jul 6, 2023

View reviewed changes

dynawaltz/src/main/java/com/powsybl/dynawaltz/models/buses/StaticDefaultBusModel.java Outdated Show resolved Hide resolved

gautierbureau approved these changes Jul 7, 2023

View reviewed changes

gautierbureau self-requested a review July 7, 2023 14:24

Use MeasurementPoint instead of ConnectionPoint in NodeFaultEvent

9619def

Signed-off-by: lisrte <[email protected]>

gautierbureau approved these changes Jul 10, 2023

View reviewed changes

Lisrte added 4 commits September 8, 2023 14:34

Merge branch 'main' into refactor_bus_models

23f2af6

Add javadoc

7b1a30b

Signed-off-by: lisrte <[email protected]>

Fix code smell

87c8a30

Signed-off-by: lisrte <[email protected]>

Fix merge main

b0269c8

Signed-off-by: lisrte <[email protected]>

gautierbureau requested a review from flo-dup October 2, 2023 09:02

Lisrte added 2 commits October 2, 2023 13:57

Fix post merge main

cbc7c65

Signed-off-by: lisrte <[email protected]>

Fix code smell

4897624

Use 'eager initialized singleton' implementation for DefaultBus Signed-off-by: lisrte <[email protected]>

flo-dup reviewed Oct 11, 2023

View reviewed changes

Renaming

bd0461c

Signed-off-by: lisrte <[email protected]>

flo-dup approved these changes Oct 13, 2023

View reviewed changes

Merge branch 'main' into refactor_bus_models

3e62dc6

Signed-off-by: Florian Dupuy <[email protected]>

flo-dup requested changes Oct 13, 2023

View reviewed changes

Lisrte added 2 commits October 18, 2023 10:17

Fix code review

918f788

Signed-off-by: lisrte <[email protected]>

Rename DefaultActionConnectionPoint and DefaultEquipmentConnectionPoi…

cbd307d

…nt getName method Signed-off-by: lisrte <[email protected]>

flo-dup approved these changes Oct 18, 2023

View reviewed changes

flo-dup merged commit f64c6d9 into main Oct 18, 2023
6 checks passed

flo-dup deleted the refactor_bus_models branch October 18, 2023 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor bus models #261

Refactor bus models #261

Lisrte commented Jul 3, 2023

gautierbureau commented Jul 4, 2023

sonarqubecloud bot commented Jul 7, 2023

flo-dup left a comment

flo-dup left a comment

flo-dup Oct 13, 2023

flo-dup Oct 13, 2023

flo-dup Oct 13, 2023

flo-dup Oct 13, 2023

gautierbureau Oct 18, 2023

flo-dup Oct 13, 2023

sonarqubecloud bot commented Oct 18, 2023

Refactor bus models #261

Refactor bus models #261

Conversation

Lisrte commented Jul 3, 2023

gautierbureau commented Jul 4, 2023

sonarqubecloud bot commented Jul 7, 2023

flo-dup left a comment

Choose a reason for hiding this comment

flo-dup left a comment

Choose a reason for hiding this comment

flo-dup Oct 13, 2023

Choose a reason for hiding this comment

flo-dup Oct 13, 2023

Choose a reason for hiding this comment

flo-dup Oct 13, 2023

Choose a reason for hiding this comment

flo-dup Oct 13, 2023

Choose a reason for hiding this comment

gautierbureau Oct 18, 2023

Choose a reason for hiding this comment

flo-dup Oct 13, 2023

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 18, 2023