add more checks for isCalculatedBusValid for updates and code improvements from reviews

Signed-off-by: HARPER Jon <[email protected]>

Author: jonenst

network-store-iidm-impl/src/main/java/com/powsybl/network/store/iidm/impl/AbstractTopology.java

@@ -362,21 +362,26 @@ private void setCalculatedBuses(Resource<VoltageLevelAttributes> voltageLevelRes
 
     private CalculatedBusAttributes findFirstMatchingNodeBreakerCalculatedBusAttributes(Resource<VoltageLevelAttributes> voltageLevelResource,
             ConnectedSetResult<T> connectedSet, boolean isBusView) {
-        // TODO Some day we may decide to start preserving values in nodebreaker topology after invalidating the views,
-        // so we could remove this check for isCalculatedBusesValid. Here it controls preserving
-        // the value accross the other view, we have it to be consistent with preserving values
-        // from the same view (currently not preserved)
-        if (voltageLevelResource.getAttributes().isCalculatedBusesValid()) {
-            List<CalculatedBusAttributes> calculatedBusAttributes = isBusView ? voltageLevelResource.getAttributes().getCalculatedBusesForBusBreakerView() : voltageLevelResource.getAttributes().getCalculatedBusesForBusView();
-            if (!CollectionUtils.isEmpty(calculatedBusAttributes)) {
-                Map<Integer, Integer> nodesToCalculatedBuses = isBusView ? voltageLevelResource.getAttributes().getNodeToCalculatedBusForBusBreakerView() : voltageLevelResource.getAttributes().getNodeToCalculatedBusForBusView();
-                if (!MapUtils.isEmpty(nodesToCalculatedBuses)) {
-                    Integer node = (Integer) connectedSet.getConnectedNodesOrBuses().iterator().next(); // non deterministic
-                    Integer busNum = nodesToCalculatedBuses.get(node);
-                    if (busNum != null) {
-                        return calculatedBusAttributes.get(busNum);
-                    }
-                }
+        // TODO Some day we may decide to start preserving phase/angle values
+        // in nodebreaker topology even after invalidating the views, so we
+        // could remove the check for isCalculatedBusesValid. Here it controls
+        // whether we preserve or not the phase/angle values accross the other
+        // view. For now we do not preserve to be consistent with the behavior
+        // of not preserving values from the same view after invalidation.
+        List<CalculatedBusAttributes> calculatedBusAttributesInOtherView = isBusView ? voltageLevelResource.getAttributes().getCalculatedBusesForBusBreakerView() : voltageLevelResource.getAttributes().getCalculatedBusesForBusView();
+        Map<Integer, Integer> nodesToCalculatedBusesInOtherView = isBusView ? voltageLevelResource.getAttributes().getNodeToCalculatedBusForBusBreakerView() : voltageLevelResource.getAttributes().getNodeToCalculatedBusForBusView();
+        Set<Integer> nodes = (Set<Integer>) connectedSet.getConnectedNodesOrBuses();
+        if (voltageLevelResource.getAttributes().isCalculatedBusesValid()
+            && !CollectionUtils.isEmpty(calculatedBusAttributesInOtherView)
+            && !MapUtils.isEmpty(nodesToCalculatedBusesInOtherView)
+            && !nodes.isEmpty()) {
+            // busNumInOtherView is deterministic for the busbreakerview because all busbreakerviewbuses correspond
+            // to the same busviewbus. For the busview, busNumInOtherView will be non deterministic, it will
+            // be one of the busbreakerbuses of this busviewbus.
+            Integer node = (Integer) nodes.iterator().next();
+            Integer busNumInOtherView = nodesToCalculatedBusesInOtherView.get(node);
+            if (busNumInOtherView != null) {
+                return calculatedBusAttributesInOtherView.get(busNumInOtherView);
             }
         }
         return null;
@@ -395,13 +400,18 @@ private CalculatedBusAttributes createCalculatedBusAttributesWithVAndAngle(Netwo
                 angle = busAttributes.getAngle();
             }
         } else { // BUS_BREAKER
-            // currently for busbreakertopology the values are always preserved
-            // when using only the busbreakerview. So mimic the behavior and always preserve them
-            // when using the busview: get V and Angle values from configured buses
-            String configuredBusId = ((ConnectedSetResult<String>) connectedSet).getConnectedNodesOrBuses().iterator().next(); // nondeterministic
-            Bus b = index.getConfiguredBus(configuredBusId).orElseThrow();
-            v = b.getV();
-            angle = b.getAngle();
+            // currently for busbreakertopology the phase/angle values are preserved
+            // when set in the busbreakerview which is in a sense always valid.
+            // So mimic the behavior and always preserve them also in the busview
+            // by *not* testing for isCalculatedBusesValid.
+            Set<String> configuredBusesIds = (Set<String>) connectedSet.getConnectedNodesOrBuses();
+            if (!configuredBusesIds.isEmpty()) {
+                // nondeterministic, chooses a random configuredbus in this busviewbus
+                String configuredBusId = configuredBusesIds.iterator().next();
+                Bus b = index.getConfiguredBus(configuredBusId).orElseThrow(IllegalStateException::new);
+                v = b.getV();
+                angle = b.getAngle();
+            }
         }
         return new CalculatedBusAttributes(connectedSet.getConnectedVertices(), null, null, v, angle);
     }

network-store-iidm-impl/src/main/java/com/powsybl/network/store/iidm/impl/CalculatedBus.java

@@ -511,27 +511,39 @@ public int getCalculatedBusNum() {
     }
 
     private void updateBusesAttributes(double value, ObjDoubleConsumer<CalculatedBusAttributes> setValue) {
-        // busnum of this bus -> nodes in this bus -> busnums in the other view -> buses of the other view
+        // Use the busnum of this bus to get the nodes in this bus to get the
+        // busnums in the other view to get the buses of the other view to
+        // update them all. For the busbreakerview, there is only one matching bus in the busview so return early.
+        // We only update when isCalculatedBusesValid is true, there is no point in updating stale bus objects and
+        // in when isCalculatedBusesValid is not true, we may even update the wrong buses (but not much of a problem
+        // because they are stale objects).
+        // TODO add tests for updates with isCalculatedBusesValid=false
+        // NOTE: we don't maintain a mapping from busnum to nodes so we iterate
+        // all the nodes and filter but it should be ok, the number is small. TODO, is this really ok ?
         VoltageLevelAttributes vlAttributes = ((VoltageLevelImpl) getVoltageLevel()).getResource().getAttributes();
         Map<Integer, Integer> nodesToCalculatedBuses = isBusView
             ? vlAttributes.getNodeToCalculatedBusForBusView()
             : vlAttributes.getNodeToCalculatedBusForBusBreakerView();
         Map<Integer, Integer> nodesToCalculatedBusesInOtherView = isBusView
             ? vlAttributes.getNodeToCalculatedBusForBusBreakerView()
             : vlAttributes.getNodeToCalculatedBusForBusView();
-        if (!MapUtils.isEmpty(nodesToCalculatedBuses) && !MapUtils.isEmpty(nodesToCalculatedBusesInOtherView)) {
+        List<CalculatedBusAttributes> calculatedBusAttributes = isBusView
+            ? vlAttributes.getCalculatedBusesForBusBreakerView()
+            : vlAttributes.getCalculatedBusesForBusView();
+        if (vlAttributes.isCalculatedBusesValid() && !CollectionUtils.isEmpty(calculatedBusAttributes)
+            && !MapUtils.isEmpty(nodesToCalculatedBuses) && !MapUtils.isEmpty(nodesToCalculatedBusesInOtherView)) {
+            Set<Integer> seen = new HashSet<>();
             for (Entry<Integer, Integer> entry : nodesToCalculatedBuses.entrySet()) {
                 if (getCalculatedBusNum() == entry.getValue()) {
                     int node = entry.getKey();
-                    if (nodesToCalculatedBusesInOtherView.containsKey(node)) {
-                        int busNumInOtherView = nodesToCalculatedBusesInOtherView.get(node);
-                        List<CalculatedBusAttributes> calculatedBusAttributes = isBusView
-                            ? vlAttributes.getCalculatedBusesForBusBreakerView()
-                            : vlAttributes.getCalculatedBusesForBusView();
-                        if (!CollectionUtils.isEmpty(calculatedBusAttributes)) {
-                            setValue.accept(calculatedBusAttributes.get(busNumInOtherView), value);
-                            index.updateVoltageLevelResource(voltageLevelResource, AttributeFilter.SV);
-                        }
+                    Integer busNumInOtherView = nodesToCalculatedBusesInOtherView.get(node);
+                    if (busNumInOtherView != null && !seen.contains(busNumInOtherView)) {
+                        setValue.accept(calculatedBusAttributes.get(busNumInOtherView), value);
+                        index.updateVoltageLevelResource(voltageLevelResource, AttributeFilter.SV);
+                        seen.add(busNumInOtherView);
+                    }
+                    if (!isBusView) {
+                        return;
                     }
                 }
             }
@@ -540,11 +552,18 @@ private void updateBusesAttributes(double value, ObjDoubleConsumer<CalculatedBus
 
     private void updateConfiguredBuses(double newValue,
                                        ObjDoubleConsumer<ConfiguredBusImpl> setValue) {
+        // update all the configured buses
+        // NOTE: we don't maintain a mapping from busnum to bus so we iterate
+        // all the buses and filter but it should be ok, the number is small. TODO, is this really ok ?
+        // We only update when isCalculatedBusesValid is true, otherwise we may update the wrong configured bus
+        // TODO add tests for updates with isCalculatedBusesValid=false
         VoltageLevelAttributes vlAttributes = ((VoltageLevelImpl) getVoltageLevel()).getResource().getAttributes();
-        for (Entry<String, Integer> entry : vlAttributes.getBusToCalculatedBusForBusView().entrySet()) {
-            if (getCalculatedBusNum() == entry.getValue()) {
-                Bus bus = getVoltageLevel().getBusBreakerView().getBus(entry.getKey());
-                setValue.accept((ConfiguredBusImpl) bus, newValue);
+        if (vlAttributes.isCalculatedBusesValid()) {
+            for (Entry<String, Integer> entry : vlAttributes.getBusToCalculatedBusForBusView().entrySet()) {
+                if (getCalculatedBusNum() == entry.getValue()) {
+                    ConfiguredBusImpl bus = index.getConfiguredBus(entry.getKey()).orElseThrow(IllegalStateException::new);
+                    setValue.accept(bus, newValue);
+                }
             }
         }
     }

network-store-iidm-impl/src/main/java/com/powsybl/network/store/iidm/impl/ConfiguredBusImpl.java

@@ -28,8 +28,10 @@
 import com.powsybl.network.store.model.AttributeFilter;
 import com.powsybl.network.store.model.CalculatedBusAttributes;
 import com.powsybl.network.store.model.ConfiguredBusAttributes;
+import com.powsybl.network.store.model.VoltageLevelAttributes;
 import com.powsybl.network.store.model.Resource;
 import org.apache.commons.collections4.MapUtils;
+import org.apache.commons.collections4.CollectionUtils;
 
 import java.util.List;
 import java.util.Map;
@@ -96,8 +98,8 @@ private void setV(double v, boolean updateCalculatedBus) {
         }
     }
 
-    // update without the part setting values in calculated buses otherwise
-    // it leads to infinite loops because calculated buses also update configured buses
+    // update without the part setting values in calculated buses otherwise it
+    // leads to infinite loops because calculated buses also update configured buses
     void setConfiguredBusV(double v) {
         setV(v, false);
     }
@@ -148,23 +150,26 @@ private void setAngleInCalculatedBus(CalculatedBusAttributes calculatedBusAttrib
     private void updateCalculatedBusAttributes(double newValue,
                                                String voltageLevelId,
                                                ObjDoubleConsumer<CalculatedBusAttributes> setValue) {
-        VoltageLevelImpl voltageLevel = index.getVoltageLevel(voltageLevelId).orElseThrow();
-        Map<String, Integer> calculatedBuses = voltageLevel.getResource().getAttributes().getBusToCalculatedBusForBusView();
-        // TODO, do we want to update the busview values if the view is invalid ?
-        // if invalid, values will be restored from the configuredbuses on the next
-        // busview computation anyway in the new bus objects and attributes,
-        // but the previous bus objects and attributes may still be used somewhere
-        // so there is a visible effect to choosing to update invalid views or not.
-        if (!MapUtils.isEmpty(calculatedBuses)) {
+        VoltageLevelImpl voltageLevel = index.getVoltageLevel(voltageLevelId).orElseThrow(IllegalArgumentException::new);
+        VoltageLevelAttributes vlAttributes = voltageLevel.getResource().getAttributes();
+        Map<String, Integer> calculatedBuses = vlAttributes.getBusToCalculatedBusForBusView();
+        List<CalculatedBusAttributes> busViewCalculatedBusesAttributes = vlAttributes.getCalculatedBusesForBusView();
+        // We only update when isCalculatedBusesValid is true, there is no point in updating stale bus objects and
+        // in when isCalculatedBusesValid is not true, we may even update the wrong buses (but not much of a problem
+        // because they are stale objects).
+        // TODO add tests for updates with isCalculatedBusesValid=false
+        if (vlAttributes.isCalculatedBusesValid()
+            && !CollectionUtils.isEmpty(busViewCalculatedBusesAttributes)
+            && !MapUtils.isEmpty(calculatedBuses)) {
             Integer busviewnum = calculatedBuses.get(getId());
             if (busviewnum != null) {
-                CalculatedBusAttributes busviewattributes = voltageLevel.getResource().getAttributes().getCalculatedBusesForBusView().get(busviewnum);
+                CalculatedBusAttributes busViewCalculatedBusAttributes = busViewCalculatedBusesAttributes.get(busviewnum);
                 // Same code as the iidm impl for CalculatedBus setV / setAngle
                 // (without the part setting values in configured buses otherwise
                 // it would be an infinite loop), but copy paste here
                 // to avoid creating the object (calculated buses are created on when computing
                 // the bus view, but we want to only update if the busview exist, not force its creation)
-                setValue.accept(busviewattributes, newValue);
+                setValue.accept(busViewCalculatedBusAttributes, newValue);
                 index.updateVoltageLevelResource(voltageLevel.getResource(), AttributeFilter.SV);
             }
         }