Fix bug on iteration within a span

The old code assumed that the Fused node only connects Fiber nodes. In a
sequence of Fused - Amplifier - Fused - Fiber, the Amplifier would be
included by a mistake. In addition, the code was not that easy to read,
and it just instantiated StopIteration without raising that (which would
be an error in a generator context). It was also rather strict, failing
if the iterator was requested for an "edge node" (a transponder), and
one of the exceptions was not actually an f-string.

Finally, the span_loss function would occasionally report wrong values
(for example in the provided test case, span_loss("fused7") would say 1
instead of 17).

Fix this by making sure that prev_node_generator() and
next_node_generator() never return anything but Fiber and Fused nodes,
which made it possible to simplify the span_loss() function. This should
now properly account for the accumulated losses of an arbitrary sequence
of Fiber and Fused elements.

I went over this a few times because set_egress_amplifier() calls
span_loss() on a *ROADM* node type. That does not make any sense, and
the code deals with this "properly" by returning a loss of 0 dB. It was
a bit confusing for me to see that it's actually OK to ask for a "span
loss" that's identified by a ROADM.

A side effect of this code is that Fused instances that are isolated
from the rest of the topology no longer raise an exception. I was
thinking about preserving this (because for GNPy, the only element with
no previous or no next nodes are the transceivers, but then Esther's
test topology contains an isolated `fused4` element. If we want to make
this strict, we can do that easily like this:

 --- a/gnpy/core/network.py
 +++ b/gnpy/core/network.py
 @@ -162,10 +162,12 @@ _fiber_fused_types = (elements.Fused, elements.Fiber)
  def prev_node_generator(network, node):
      """fused spans interest:
      iterate over all predecessors while they are Fused or Fiber type"""
      try:
          prev_node = next(network.predecessors(node))
      except StopIteration:
 -        return
 +        if isinstance(node, elements.Transceiver):
 +            return
 +        raise NetworkTopologyError(f'Node {node.uid} is not properly connected, please check network topology')
      if isinstance(prev_node, _fiber_fused_types) and isinstance(node, _fiber_fused_types):
          yield prev_node
          yield from prev_node_generator(network, prev_node)

Signed-off-by: EstherLerouzic <esther.lerouzic@orange.com>
Co-authored-by: Jan Kundrát <jan.kundrat@telecominfraproject.com>
Change-Id: I41a65e89eef763f82e41e52bc325ed2d488cb601

gnpy/core/network.py

@@ -156,19 +156,19 @@ def target_power(network, node, equipment):  # get_fiber_dp
     return dp
 
 
+_fiber_fused_types = (elements.Fused, elements.Fiber)
+
+
 def prev_node_generator(network, node):
     """fused spans interest:
     iterate over all predecessors while they are Fused or Fiber type"""
     try:
         prev_node = next(network.predecessors(node))
     except StopIteration:
-        raise NetworkTopologyError(f'Node {node.uid} is not properly connected, please check network topology')
-    # yield and re-iterate
-    if isinstance(prev_node, elements.Fused) or isinstance(node, elements.Fused):
+        return
+    if isinstance(prev_node, _fiber_fused_types) and isinstance(node, _fiber_fused_types):
         yield prev_node
         yield from prev_node_generator(network, prev_node)
-    else:
-        StopIteration
 
 
 def next_node_generator(network, node):
@@ -177,31 +177,17 @@ def next_node_generator(network, node):
     try:
         next_node = next(network.successors(node))
     except StopIteration:
-        raise NetworkTopologyError('Node {node.uid} is not properly connected, please check network topology')
-    # yield and re-iterate
-    if isinstance(next_node, elements.Fused) or isinstance(node, elements.Fused):
+        return
+    if isinstance(next_node, _fiber_fused_types) and isinstance(node, _fiber_fused_types):
         yield next_node
         yield from next_node_generator(network, next_node)
-    else:
-        StopIteration
 
 
 def span_loss(network, node):
-    """Fused span interest:
-    return the total span loss of all the fibers spliced by a Fused node"""
+    """Total loss of a span (Fiber and Fused nodes) which contains the given node"""
     loss = node.loss if node.passive else 0
-    try:
-        prev_node = next(network.predecessors(node))
-        if isinstance(prev_node, elements.Fused):
-            loss += sum(n.loss for n in prev_node_generator(network, node))
-    except StopIteration:
-        pass
-    try:
-        next_node = next(network.successors(node))
-        if isinstance(next_node, elements.Fused):
-            loss += sum(n.loss for n in next_node_generator(network, node))
-    except StopIteration:
-        pass
+    loss += sum(n.loss for n in prev_node_generator(network, node))
+    loss += sum(n.loss for n in next_node_generator(network, node))
     return loss
 
 

tests/data/bugfixiteratortopo.json

@@ -0,0 +1,392 @@
+{
+  "elements": [
+    {
+      "uid": "Site_A",
+      "type": "Transceiver",
+      "metadata": {
+        "location": {
+          "latitude": 0,
+          "longitude": 0,
+          "city": "Site A",
+          "region": ""
+        }
+      }
+    },
+    {
+      "uid": "nodeA",
+      "type": "Roadm",
+      "params": {
+        "target_pch_out_db": -18,
+        "restrictions": {
+          "preamp_variety_list": [],
+          "booster_variety_list": []
+        },
+        "per_degree_pch_out_db": {}
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0.0,
+          "longitude": 0.0,
+          "city": "Zion",
+          "region": ""
+        }
+      }
+    },
+    {
+      "uid": "fiber1",
+      "type": "Fiber",
+      "type_variety": "SSMF",
+      "params": {
+        "length": 50.0,
+        "loss_coef": 0.2,
+        "length_units": "km",
+        "att_in": 0,
+        "con_in": 0.0,
+        "con_out": 0.0
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0,
+          "longitude": 0,
+          "city": null,
+          "region": null
+        }
+      }
+    },
+    {
+      "uid": "fused1",
+      "type": "Fused",
+      "params": {
+        "loss": 0.0
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0,
+          "longitude": 0,
+          "city": null,
+          "region": null
+        }
+      }
+    },
+    {
+      "uid": "fiber2",
+      "type": "Fiber",
+      "type_variety": "SSMF",
+      "params": {
+        "length": 1.0,
+        "loss_coef": 0.5,
+        "length_units": "km",
+        "att_in": 0,
+        "con_in": 0.0,
+        "con_out": 0.0
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0,
+          "longitude": 0,
+          "city": null,
+          "region": null
+        }
+      }
+    },
+    {
+      "uid": "amp2",
+      "type": "Edfa",
+      "type_variety": "std_medium_gain",
+      "metadata": {
+        "location": {
+          "latitude": 0.0,
+          "longitude": 0.0,
+          "city": "Zion",
+          "region": ""
+        }
+      }
+    },
+    {
+      "uid": "fiber3",
+      "type": "Fiber",
+      "type_variety": "SSMF",
+      "params": {
+        "length": 80.0,
+        "loss_coef": 0.2,
+        "length_units": "km",
+        "att_in": 0,
+        "con_in": 0.0,
+        "con_out": 0.0
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0,
+          "longitude": 0,
+          "city": null,
+          "region": null
+        }
+      }
+    },
+    {
+      "uid": "fused7",
+      "type": "Fused",
+      "params": {
+        "loss": 1.0
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0,
+          "longitude": 0,
+          "city": null,
+          "region": null
+        }
+      }
+    },
+    {
+      "uid": "fiber6",
+      "type": "Fiber",
+      "type_variety": "SSMF",
+      "params": {
+        "length": 80.0,
+        "loss_coef": 0.2,
+        "length_units": "km",
+        "att_in": 0,
+        "con_in": 0.0,
+        "con_out": 0.0
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0,
+          "longitude": 0,
+          "city": null,
+          "region": null
+        }
+      }
+    },
+    {
+      "uid": "nodeC",
+      "type": "Roadm",
+      "params": {
+        "target_pch_out_db": -18,
+        "restrictions": {
+          "preamp_variety_list": [],
+          "booster_variety_list": []
+        },
+        "per_degree_pch_out_db": {}
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0.0,
+          "longitude": 0.0,
+          "city": "Zion",
+          "region": ""
+        }
+      }
+    },
+    {
+      "uid": "Site_C",
+      "type": "Transceiver",
+      "metadata": {
+        "location": {
+          "latitude": 0,
+          "longitude": 0,
+          "city": "Site A",
+          "region": ""
+        }
+      }
+    },
+    {
+      "uid": "amp3",
+      "type": "Edfa",
+      "type_variety": "std_medium_gain",
+      "metadata": {
+        "location": {
+          "latitude": 0.0,
+          "longitude": 0.0,
+          "city": "Zion",
+          "region": ""
+        }
+      }
+    },
+    {
+      "uid": "fused4",
+      "type": "Fused",
+      "params": {
+        "loss": 1.0
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0,
+          "longitude": 0,
+          "city": null,
+          "region": null
+        }
+      }
+    },
+    {
+      "uid": "fiber4",
+      "type": "Fiber",
+      "type_variety": "SSMF",
+      "params": {
+        "length": 80.0,
+        "loss_coef": 0.2,
+        "length_units": "km",
+        "att_in": 0,
+        "con_in": 0.0,
+        "con_out": 0.0
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0,
+          "longitude": 0,
+          "city": null,
+          "region": null
+        }
+      }
+    },
+    {
+      "uid": "amp4",
+      "type": "Edfa",
+      "type_variety": "std_medium_gain",
+      "operational": {
+        "gain_target": 13.31,
+        "delta_p": 1.18,
+        "tilt_target": -0.81,
+        "out_voa": 2.0
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0.0,
+          "longitude": 0.0,
+          "city": "Zion",
+          "region": ""
+        }
+      }
+    },
+    {
+      "uid": "fused5",
+      "type": "Fused",
+      "params": {
+        "loss": 0.0
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0,
+          "longitude": 0,
+          "city": null,
+          "region": null
+        }
+      }
+    },
+    {
+      "uid": "nodeB",
+      "type": "Roadm",
+      "params": {
+        "target_pch_out_db": -15,
+        "restrictions": {
+          "preamp_variety_list": [],
+          "booster_variety_list": []
+        },
+        "per_degree_pch_out_db": {}
+      },
+      "metadata": {
+        "location": {
+          "latitude": 0.0,
+          "longitude": 0.0,
+          "city": "Zion",
+          "region": ""
+        }
+      }
+    },
+    {
+      "uid": "Site_B",
+      "type": "Transceiver",
+      "metadata": {
+        "location": {
+          "latitude": 2,
+          "longitude": 0,
+          "city": "Site B",
+          "region": ""
+        }
+      }
+    }
+  ],
+  "connections": [
+    {
+      "from_node": "Site_A",
+      "to_node": "nodeA"
+    },
+    {
+      "from_node": "nodeA",
+      "to_node": "Site_A"
+    },
+    {
+      "from_node": "nodeA",
+      "to_node": "fiber1"
+    },
+    {
+      "from_node": "fiber1",
+      "to_node": "fused1"
+    },
+    {
+      "from_node": "fused1",
+      "to_node": "fiber2"
+    },
+    {
+      "from_node": "fiber2",
+      "to_node": "amp2"
+    },
+    {
+      "from_node": "amp2",
+      "to_node": "fiber3"
+    },
+    {
+      "from_node": "fiber3",
+      "to_node": "nodeC"
+    },
+    {
+      "from_node": "nodeC",
+      "to_node": "Site_C"
+    },
+    {
+      "from_node": "Site_C",
+      "to_node": "nodeC"
+    },
+    {
+      "from_node": "nodeC",
+      "to_node": "amp3"
+    },
+    {
+      "from_node": "amp3",
+      "to_node": "fiber4"
+    },
+    {
+      "from_node": "fiber4",
+      "to_node": "amp4"
+    },
+    {
+      "from_node": "amp4",
+      "to_node": "fused5"
+    },
+    {
+      "from_node": "fused5",
+      "to_node": "nodeB"
+    },
+    {
+      "from_node": "nodeB",
+      "to_node": "fused7"
+    },
+    {
+      "from_node": "fused7",
+      "to_node": "fiber6"
+    },
+    {
+      "from_node": "fiber6",
+      "to_node": "nodeA"
+    },
+    {
+      "from_node": "Site_B",
+      "to_node": "nodeB"
+    },
+    {
+      "from_node": "nodeB",
+      "to_node": "Site_B"
+    }
+  ]
+}

tests/test_network_functions.py

@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: BSD-3-Clause
+#
+# Copyright (C) 2020 Telecom Infra Project and GNPy contributors
+# see LICENSE.md for a list of contributors
+#
+
+from pathlib import Path
+import pytest
+from gnpy.core.network import span_loss
+from gnpy.tools.json_io import load_equipment, load_network
+
+
+TEST_DIR = Path(__file__).parent
+EQPT_FILENAME = TEST_DIR / 'data/eqpt_config.json'
+NETWORK_FILENAME = TEST_DIR / 'data/bugfixiteratortopo.json'
+
+
+@pytest.mark.parametrize("node, attenuation", [
+    # first fiber span
+    ['fiber1', 10.5],
+    ['fiber2', 10.5],
+    ['fused1', 10.5],
+    # second span
+    ['fiber3', 16.0],
+    # third span
+    ['fiber4', 16.0],
+    # direct link between a ROADM and an amplifier
+    ['fused5', 0],
+    # fourth span
+    ['fiber6', 17],
+    ['fused7', 17],
+    # not connected anywhere
+    ['fused4', 1],
+    # all other nodes
+    ['Site_A', 0],
+    ['nodeA', 0],
+    ['amp2', 0],
+    ['nodeC', 0],
+    ['Site_C', 0],
+    ['amp3', 0],
+    ['amp4', 0],
+    ['nodeB', 0],
+    ['Site_B', 0],
+])
+def test_span_loss(node, attenuation):
+    equipment = load_equipment(EQPT_FILENAME)
+    network = load_network(NETWORK_FILENAME, equipment)
+    for x in network.nodes():
+        if x.uid == node:
+            assert attenuation == span_loss(network, x)
+            return
+    assert not f'node "{node}" referenced from test but not found in the topology'  # pragma: no cover