From 9b91bd85b7f3036aa958154bd9991f5952aee25c Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Tue, 11 Aug 2026 10:36:55 +0200 Subject: [PATCH 1/2] Reach EPANET link quantities via duplicated end breakpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A link-node reach (EPANET) has one synthetic gridpoint belonging to neither end, so its own quantities — Flow on a pipe, energy on a pump — were unreachable through find() or ReachObservation. Duplicate that gridpoint into a breakpoint at each end: distance 0.0, and the reach length where known. Reaches with real gridpoints are unaffected. A companion .resx now contributes reach-level quantities the same way it already did for nodes, matched by gridpoint index, so _merge_extra_quantities takes a location_id rather than a node_id. Where the length is unknown the trailing breakpoint's distance is None, so ReachBreakPoint.distance widens to float | None and the graph's distance arithmetic guards against it. Such a breakpoint is not addressable by find(reach=..., distance=...), but stays reachable via ReachObservation and recall(). Co-Authored-By: Claude Opus 5 --- docs/user-guide/network.qmd | 9 +- src/modelskill/model/adapters/_res1d.py | 124 ++++++++++++------ src/modelskill/network.py | 116 +++++++++++------ tests/test_network.py | 163 ++++++++++++++++++++++-- 4 files changed, 312 insertions(+), 100 deletions(-) diff --git a/docs/user-guide/network.qmd b/docs/user-guide/network.qmd index d22f0c1c6..384b9a4e7 100644 --- a/docs/user-guide/network.qmd +++ b/docs/user-guide/network.qmd @@ -226,13 +226,12 @@ sorted( ::: {.callout-warning} ## EPANET reach geometry is limited -EPANET is a link-node model, and mikeio1d reports no length and a single synthetic gridpoint for each reach. So for an EPANET network: +EPANET is a link-node model, and mikeio1d reports a single synthetic gridpoint for each reach, not tied to either end. That gridpoint is duplicated into two breakpoints, one at each end of the reach, so its own quantities (`Flow`, `Velocity`, ...) reach `find()`/`recall()`/`ReachObservation` the same way a MIKE reach's end data already does. So for an EPANET network: -* without `inp=`, every edge of `network.graph` has `length=None`. A length-weighted `networkx` call then fails rather than returning a meaningless number — shortest-path treats the edge as unreachable, and anything that sums the weights raises `TypeError`. The attribute is always present, since `networkx` defaults a missing weight to `1`. With `inp=`, only pumps and valves stay `None`, since `[PIPES]` is the one section carrying lengths -* reaches have no breakpoints, so a `ReachObservation` cannot be matched — use `NodeObservation` instead -* `find(reach=..., distance=)` never resolves; only `distance="start"` and `distance="end"` work +* without `inp=`, a reach's length is unknown, so only its first breakpoint (`distance=0.0`) is real; the second isn't addressable by a number at all — `find(reach=..., distance=)` only resolves it via `distance="start"`/`"end"` (which return the node, not the breakpoint). The corresponding edges of `network.graph` are `length=None` — a length-weighted `networkx` call then fails rather than returning a meaningless number, since shortest-path treats a `None`-weight edge as unreachable and anything that sums the weights raises `TypeError` +* with `inp=`, a pipe's second breakpoint sits at its full length, and the edge between the two breakpoints carries that real length. Pumps and valves keep an unaddressable second breakpoint even with `inp=`, since `[PIPES]` is the only section carrying lengths -For the same reason, `resx=` merges node quantities only. Its reach-level quantities — pump energy, efficiency and costs — have no breakpoint to live on, which is tracked in [#680](https://github.com/DHI/modelskill/issues/680). +`resx=`'s reach-level quantities — pump energy, efficiency and costs — merge onto the matching reach's breakpoints the same way its node quantities merge onto nodes. Node timeseries, `to_dataframe()`, `to_dataset()`, `find(node=...)` and `recall()` are unaffected. ::: diff --git a/src/modelskill/model/adapters/_res1d.py b/src/modelskill/model/adapters/_res1d.py index aea5dfd6d..22d8f9077 100644 --- a/src/modelskill/model/adapters/_res1d.py +++ b/src/modelskill/model/adapters/_res1d.py @@ -5,6 +5,7 @@ import pandas as pd if TYPE_CHECKING: + from mikeio1d import Res1D from mikeio1d.result_network import ResultNode, ResultGridPoint, ResultReach from modelskill.network import NetworkNode, ReachBreakPoint, NetworkReach @@ -63,18 +64,18 @@ def _simplify_colnames( def _merge_extra_quantities( - base: pd.DataFrame, extra: pd.DataFrame, *, node_id: str + base: pd.DataFrame, extra: pd.DataFrame, *, location_id: str ) -> pd.DataFrame: - """Append a companion file's quantities to a node's frame as extra columns. + """Append a companion file's quantities to a node's or reach's frame. Parameters ---------- base : pd.DataFrame - The node's frame from the main result file. + The node's or reach's frame from the main result file. extra : pd.DataFrame - The same node's frame from the companion file, sharing its time index. - node_id : str - Node ID, used in error messages. + The same location's frame from the companion file, sharing its time index. + location_id : str + Node or reach ID, used in error messages. Returns ------- @@ -83,9 +84,9 @@ def _merge_extra_quantities( Raises ------ ValueError - If a quantity appears in both frames. Concatenating would give the node - two columns of the same name, which is the state ``_simplify_colnames`` - already refuses. + If a quantity appears in both frames. Concatenating would give the + location two columns of the same name, which is the state + ``_simplify_colnames`` already refuses. """ if extra.empty: return base @@ -93,8 +94,8 @@ def _merge_extra_quantities( overlapping = base.columns.intersection(extra.columns) if len(overlapping) > 0: raise ValueError( - f"Node {node_id!r} already has {sorted(overlapping)} in the main " - "result file, so the companion file's copy cannot be merged in." + f"Location {location_id!r} already has {sorted(overlapping)} in the " + "main result file, so the companion file's copy cannot be merged in." ) return pd.concat([base, extra], axis=1) @@ -121,13 +122,13 @@ def data(self) -> pd.DataFrame: class GridPoint(ReachBreakPoint): def __init__( - self, reach_id: str, chainage: float, data: pd.DataFrame | None = None + self, reach_id: str, chainage: float | None, data: pd.DataFrame | None = None ): self._id = (reach_id, chainage) self._data = _EMPTY_DATA if data is None else data @property - def id(self) -> tuple[str, float]: + def id(self) -> tuple[str, float | None]: return self._id @property @@ -135,6 +136,71 @@ def data(self) -> pd.DataFrame: return self._data +def _resolve_reach_length(length: float | None, reach: ResultReach) -> float | None: + """Resolve a reach's effective length. + + A length read from a companion input file wins, since mikeio1d has none + to offer for the formats that need one. Otherwise: mikeio1d returns 0 + when it cannot read a reach length - link-node models such as EPANET + report this for every reach. Report it as undefined rather than as a + zero-length reach, which would make length-weighted graph algorithms + treat the reach as free. The two cases cannot be told apart upstream. + """ + return length if length is not None else (reach.length or None) + + +def _build_reach_breakpoints( + reach: ResultReach, + *, + length: float | None, + quantities: set[str] | None, + populate_gridpoints: bool, + extra: Res1D | None = None, +) -> list[ReachBreakPoint]: + """Build a reach's break points from its mikeio1d gridpoints. + + Reaches with more than 2 gridpoints have real, independently-measured + start/end points, so every gridpoint becomes a break point at its own + chainage (the first/last ones end up coincident with the reach's own + start_node/end_node - Network._generate_graph connects them with a + zero-length edge). + + Reaches with 2 or fewer gridpoints are link-node models (e.g. EPANET), + whose single synthetic gridpoint belongs to neither end - it is + duplicated into two break points, one at each end (distance 0.0, and + distance `length` if known or None otherwise), so the reach's own + quantities (e.g. Flow) are reachable the same way MIKE's are. See + https://github.com/DHI/modelskill/issues/680. + + A companion ``.resx`` result (``extra``) contributes its own reach-level + quantities (e.g. pump energy) the same way it already does for nodes, + matched to the main file's gridpoints by index - the only real case + today is a single-gridpoint reach against a single-gridpoint companion. + """ + if len(reach.gridpoints) > 2: + unique_gridpoints = reach.gridpoints + distances_per_gridpoint = [[gp.chainage] for gp in unique_gridpoints] + else: + unique_gridpoints = reach.gridpoints[:1] + distances_per_gridpoint = [[0.0, length] for _ in unique_gridpoints] + + extra_gridpoints: list[ResultGridPoint] = [] + if extra is not None and reach.name in extra.reaches: + extra_gridpoints = extra.reaches[reach.name].gridpoints + + breakpoints: list[ReachBreakPoint] = [] + for i, (gp, distances) in enumerate(zip(unique_gridpoints, distances_per_gridpoint)): + data = _simplify_colnames(gp, quantities) if populate_gridpoints else None + if data is not None and i < len(extra_gridpoints): + data = _merge_extra_quantities( + data, + _simplify_colnames(extra_gridpoints[i], quantities), + location_id=reach.name, + ) + breakpoints.extend(GridPoint(gp.reach_name, d, data) for d in distances) + return breakpoints + + class Res1DReach(NetworkReach): """NetworkReach adapter for a mikeio1d ResultReach.""" @@ -144,9 +210,8 @@ def __init__( start_node: Res1DNode, end_node: Res1DNode, *, - populate_gridpoints: bool = True, length: float | None = None, - quantities: set[str] | None = None, + breakpoints: list[ReachBreakPoint] | None = None, ): self._id = reach.name @@ -163,35 +228,10 @@ def __init__( if end_node.id != reach.end_node: raise ValueError("Incorrect ending node.") - # Reaches with more than 2 gridpoints have real, independently-measured - # start/end points, so the first and last gridpoint become breakpoints - # too (coincident with start_node/end_node - Network._generate_graph - # connects them with a zero-length edge). Reaches with 2 or fewer - # gridpoints are link-node models (e.g. EPANET), whose single synthetic - # gridpoint belongs to neither end; that case is not handled here, see - # https://github.com/DHI/modelskill/issues/680. - breakpoint_gridpoints = reach.gridpoints if len(reach.gridpoints) > 2 else [] - self._start = start_node self._end = end_node - - # A length read from a companion input file wins, since mikeio1d has none - # to offer for the formats that need one. Otherwise: mikeio1d returns 0 - # when it cannot read a reach length - link-node models such as EPANET - # report this for every reach. Report it as undefined rather than as a - # zero-length reach, which would make length-weighted graph algorithms - # treat the reach as free. The two cases cannot be told apart upstream. - self._length = length if length is not None else (reach.length or None) - self._breakpoints: list[ReachBreakPoint] = [ - GridPoint( - gridpoint.reach_name, - gridpoint.chainage, - _simplify_colnames(gridpoint, quantities) - if populate_gridpoints - else None, - ) - for gridpoint in breakpoint_gridpoints - ] + self._length = _resolve_reach_length(length, reach) + self._breakpoints = breakpoints or [] @property def id(self) -> str: diff --git a/src/modelskill/network.py b/src/modelskill/network.py index 0938a1008..0bad60450 100644 --- a/src/modelskill/network.py +++ b/src/modelskill/network.py @@ -142,12 +142,17 @@ class ReachBreakPoint(ABC): Two properties must be implemented: * :attr:`id` - a ``(reach_id, distance)`` tuple that uniquely locates the - break point within the network. + break point within the network. ``distance`` may be ``None`` when the + break point's position along the reach is genuinely unknown (e.g. a + link-node reach with no known length). * :attr:`data` - a time-indexed :class:`pandas.DataFrame` whose columns are quantity names. The :attr:`distance` convenience property returns ``id[1]`` (the - along-reach distance in the units used by the parent network). + along-reach distance in the units used by the parent network, or + ``None`` if unknown). A break point with an unknown distance cannot be + looked up via ``find(reach=..., distance=)``, but is still + reachable through ``ReachObservation`` and ``recall()``. Examples -------- @@ -171,7 +176,7 @@ class ReachBreakPoint(ABC): @property @abstractmethod - def id(self) -> tuple[str, float]: + def id(self) -> tuple[str, float | None]: """``(reach_id, distance)`` tuple uniquely identifying this break point.""" pass @@ -182,8 +187,8 @@ def data(self) -> pd.DataFrame: pass @property - def distance(self) -> float: - """Along-reach distance of this break point, measured from the start node.""" + def distance(self) -> float | None: + """Along-reach distance from the start node, or None if unknown.""" return self.id[1] @property @@ -536,8 +541,10 @@ def from_epanet( resx : str, Path, Res1D or None, optional Companion ``.resx`` file from the same run. Its extra node quantities (tank ``Volume`` and ``Volume Percentage``) are merged - onto the matching nodes. By default None, and those quantities are - simply absent. + onto the matching nodes, and its extra reach quantities (e.g. pump + ``efficiency``, ``energy`` and ``energy costs``) are merged onto + the matching reach's breakpoints. By default None, and those + quantities are simply absent. inp : str, Path or None, optional EPANET ``.inp`` input file for the same model, read for its ``[PIPES]`` lengths. By default None, and reach lengths are @@ -545,9 +552,10 @@ def from_epanet( nodes : str, list of str, or None, optional Which nodes get their timeseries loaded. See :meth:`from_mike`. reaches : str, list of str, or None, optional - Which reaches get their gridpoint data loaded. See - :meth:`from_mike`. EPANET results have no intermediate gridpoints, - so this argument has no effect. + Which reaches get their breakpoint data loaded. See + :meth:`from_mike`. EPANET reaches have at most one gridpoint (see + Notes), but this argument still governs whether its data - and + any matching ``resx`` reach quantities - are populated. quantities : str, list of str, or None, optional Which quantities are read at each selected location. See :meth:`from_mike`. @@ -580,22 +588,29 @@ def from_epanet( Notes ----- - EPANET is a link-node model, and mikeio1d reports no length and a - single synthetic gridpoint for each of its reaches. As a result: - - * without ``inp``, every edge of :attr:`graph` has ``length=None``, so a - length-weighted graph algorithm fails rather than returning a - meaningless number. Pumps and valves keep ``length=None`` even with - ``inp``, since ``[PIPES]`` is the only section carrying lengths - * reaches have no breakpoints, so - :class:`~modelskill.obs.ReachObservation` cannot be matched against - an EPANET network — use :class:`~modelskill.obs.NodeObservation` - * ``find(reach=..., distance=)`` never resolves; only - ``distance="start"`` and ``distance="end"`` work - - For the same reason, ``resx`` merges node quantities only. Its - reach-level quantities (pump energy, efficiency and costs) have no - breakpoint to live on, which is tracked in issue #680. + EPANET is a link-node model, and mikeio1d reports a single synthetic + gridpoint for each reach, not tied to either end. That gridpoint is + duplicated into two breakpoints, one at each end of the reach, so its + own quantities (``Flow``, ``Velocity``, ...) are reachable through + :meth:`find`, :meth:`recall` and + :class:`~modelskill.obs.ReachObservation` the same way a MIKE reach's + end data already is. As a result: + + * without ``inp``, a reach's length is unknown, so only its first + breakpoint (``distance=0.0``) is real; the second is not + addressable by distance at all — ``find(reach=..., distance=...)`` + resolves it only via ``distance="start"``/``"end"`` (which return + the node, not the breakpoint), or not at all by a number. The + corresponding edges of :attr:`graph` are ``length=None`` + * with ``inp``, a pipe's second breakpoint sits at its full length — + both breakpoints are then addressable by distance, and the edge + between them carries the pipe's real length. Pumps and valves keep + an unaddressable second breakpoint even with ``inp``, since + ``[PIPES]`` is the only section carrying lengths + + ``resx``'s reach-level quantities (pump ``efficiency``, ``energy`` and + ``energy costs``) merge onto the matching reach's breakpoints the same + way its node quantities merge onto nodes. Node timeseries, :meth:`to_dataframe`, :meth:`to_dataset`, ``find(node=...)`` and :meth:`recall` are unaffected. @@ -829,7 +844,9 @@ def _load_res1d_network( from modelskill.model.adapters._res1d import ( Res1DReach, Res1DNode, + _build_reach_breakpoints, _merge_extra_quantities, + _resolve_reach_length, _simplify_colnames, ) @@ -856,24 +873,31 @@ def _init_node(reach: ResultReach, is_end: bool) -> Res1DNode: df = _merge_extra_quantities( df, _simplify_colnames(extra.nodes[id], quantities), - node_id=id, + location_id=id, ) node_data[id] = df return Res1DNode(id, data=node_data[id]) else: return Res1DNode(id) - return [ - Res1DReach( + def _build_reach(reach: ResultReach) -> Res1DReach: + reach_length = lengths.get(reach.name) + breakpoints = _build_reach_breakpoints( + reach, + length=_resolve_reach_length(reach_length, reach), + quantities=quantities, + populate_gridpoints=reach.name in reaches_set, + extra=extra, + ) + return Res1DReach( reach, _init_node(reach, False), _init_node(reach, True), - populate_gridpoints=reach.name in reaches_set, - length=lengths.get(reach.name), - quantities=quantities, + length=reach_length, + breakpoints=breakpoints, ) - for reach in res.reaches.values() - ] + + return [_build_reach(reach) for reach in res.reaches.values()] @staticmethod def _generate_alias_map(g: nx.Graph) -> dict[str | tuple[str, float], int]: @@ -983,10 +1007,17 @@ def _generate_graph(reaches: Sequence[NetworkReach]) -> nx.Graph: # each derived from a different upstream source than the other # endpoint's coordinate, so floating-point noise could otherwise # leave a spurious tiny positive or negative edge weight where - # the true value is analytically zero. + # the true value is analytically zero. A breakpoint's distance + # can also be genuinely unknown (unrelated to whether the + # reach's own length is known - a NetworkReach subclass makes + # no promise the two are coupled), so both ends guard for None. leading_distance = reach.breakpoints[0].distance - leading_is_boundary = abs(leading_distance) <= _CHAINAGE_TOLERANCE - leading_length = 0.0 if leading_is_boundary else leading_distance + if leading_distance is None: + leading_length = None + leading_is_boundary = False + else: + leading_is_boundary = abs(leading_distance) <= _CHAINAGE_TOLERANCE + leading_length = 0.0 if leading_is_boundary else leading_distance g0.add_edge( start_key, bp_keys[0], @@ -998,11 +1029,12 @@ def _generate_graph(reaches: Sequence[NetworkReach]) -> nx.Graph: # distances are known even when the total is not, so a reach # without a length still gets real lengths on every edge but # this one. - if reach.length is None: + trailing_distance = reach.breakpoints[-1].distance + if reach.length is None or trailing_distance is None: tail_length = None tail_is_boundary = False else: - tail_diff = reach.length - reach.breakpoints[-1].distance + tail_diff = reach.length - trailing_distance tail_is_boundary = abs(tail_diff) <= _CHAINAGE_TOLERANCE tail_length = 0.0 if tail_is_boundary else tail_diff g0.add_edge( @@ -1016,7 +1048,10 @@ def _generate_graph(reaches: Sequence[NetworkReach]) -> nx.Graph: for i in range(reach.n_breakpoints - 1): current_ = reach.breakpoints[i] next_ = reach.breakpoints[i + 1] - length = next_.distance - current_.distance + if current_.distance is None or next_.distance is None: + length = None + else: + length = next_.distance - current_.distance g0.add_edge( current_.id, next_.id, @@ -1165,6 +1200,7 @@ def _resolve_id(id): if ( isinstance(key, tuple) and key[0] == reach_id + and key[1] is not None and abs(key[1] - distance) <= _CHAINAGE_TOLERANCE ): return val diff --git a/tests/test_network.py b/tests/test_network.py index 30db67a8e..07e8962a7 100644 --- a/tests/test_network.py +++ b/tests/test_network.py @@ -19,6 +19,7 @@ from modelskill.model.adapters._res1d import ( Res1DNode, Res1DReach, + _merge_extra_quantities, _simplify_colnames, ) from modelskill.network import ( @@ -1352,6 +1353,28 @@ def test_real_length_passes_through(self): assert reach.length == 47.5 +class TestMergeExtraQuantities: + """_merge_extra_quantities serves both node and reach loading. + + Its error message is format-neutral ("Location", not "Node") since it + no longer knows whether location_id names a node or a reach. + """ + + def test_collision_error_names_the_location(self): + base = pd.DataFrame({"Flow": [1.0]}) + extra = pd.DataFrame({"Flow": [2.0]}) + + with pytest.raises(ValueError, match=r"Location '42'"): + _merge_extra_quantities(base, extra, location_id="42") + + def test_collision_error_reports_the_overlapping_columns(self): + base = pd.DataFrame({"Flow": [1.0], "Velocity": [1.0]}) + extra = pd.DataFrame({"Flow": [2.0]}) + + with pytest.raises(ValueError, match=r"\['Flow'\]"): + _merge_extra_quantities(base, extra, location_id="9") + + # --------------------------------------------------------------------------- # from_mike / from_epanet # --------------------------------------------------------------------------- @@ -1452,32 +1475,74 @@ class TestFromEpanet: def test_epanet(self): network = Network.from_epanet("./tests/testdata/epanet.res") - assert network.graph.number_of_nodes() == 11 + # 11 topology nodes, plus 2 duplicated breakpoints per reach (13 reaches). + assert network.graph.number_of_nodes() == 11 + 2 * 13 assert len(network._reaches) == 13 assert set(network.quantities) == { "Demand", "Head", "Pressure", "WaterQuality", + "Flow", + "Velocity", + "HeadlossPer1000Unit", + "AvgWaterQuality", + "StatusCode", + "Setting", + "ReactorRate", + "FrictionFactor", } assert not network.to_dataframe().empty - def test_link_node_reaches_have_no_length_or_breakpoints(self): - """Without inp=, mikeio1d reports neither - documented in the docstring.""" + def test_link_node_reaches_get_two_breakpoints(self): + """The single synthetic gridpoint is duplicated to both ends of the reach. + + Without inp=, reach.length is unknown, so only the leading edge (at + the reach's start, distance 0.0) is real; the connecting edge and the + trailing edge are both undefined - documented in the docstring. + """ network = Network.from_epanet("./tests/testdata/epanet.res") + assert all(r.n_breakpoints == 2 for r in network._reaches.values()) + + for reach in network._reaches.values(): + assert reach.breakpoints[0].distance == 0.0 + assert reach.breakpoints[1].distance is None + lengths = [d["length"] for *_, d in network.graph.edges(data=True)] - assert lengths and all(length is None for length in lengths) - assert all(r.n_breakpoints == 0 for r in network._reaches.values()) + boundary_lengths = [ + d["length"] for *_, d in network.graph.edges(data=True) if d["boundary"] + ] + assert len(lengths) == 3 * 13 + assert boundary_lengths == [0.0] * 13 + assert sum(length is None for length in lengths) == 2 * 13 - def test_reach_observation_cannot_be_matched(self, sample_node_data): - """Follows from having no breakpoints; also documented in the docstring.""" + def test_reach_observation_matches_via_the_duplicated_breakpoint( + self, sample_node_data + ): + """A reach's Flow quantity is now reachable through its breakpoints.""" network = Network.from_epanet("./tests/testdata/epanet.res") - nmr = NetworkModelResult(network, item="Pressure") - obs = ms.ReachObservation(sample_node_data, reach="10", item="WaterLevel") + nmr = NetworkModelResult(network, item="Flow", name="epanet_model") + obs_data = sample_node_data.rename(columns={"WaterLevel": "Flow"}) + obs = ms.ReachObservation(obs_data, reach="10", item="Flow") - with pytest.raises(ValueError, match="breakpoints"): - nmr.extract(obs) + extracted = nmr.extract(obs) + + assert isinstance(extracted, NodeModelResult) + assert extracted.name == "epanet_model" + + def test_find_reach_end_breakpoint_with_unknown_length_is_not_addressable(self): + """A None-distance breakpoint can't be found by a numeric distance. + + This must raise the normal KeyError, not crash inside find()'s + tolerance-matching loop just because some breakpoint's distance is + unknown. + """ + network = Network.from_epanet("./tests/testdata/epanet.res") + assert network._reaches["10"].breakpoints # guard against a vacuous check + + with pytest.raises(KeyError): + network.find(reach="10", distance=100.0) def test_mike_file_is_redirected(self): with pytest.raises(ValueError, match=r"Use Network\.from_mike\(\)"): @@ -1531,8 +1596,38 @@ def test_pump_reach_stays_undefined(self): def test_graph_edges_carry_the_lengths(self): network = Network.from_epanet(_EPANET_RES, inp=_EPANET_INP) + # 12 pipes x 3 real edges (leading 0.0, the pipe's full length, trailing + # 0.0) + the pump's 1 real edge (leading 0.0; its trailing/connecting + # edges stay None since its own length is unknown). lengths = [d["length"] for *_, d in network.graph.edges(data=True)] - assert sum(v is not None for v in lengths) == 12 + assert len(lengths) == 13 * 3 + assert sum(v is not None for v in lengths) == 12 * 3 + 1 + + def test_pipe_breakpoints_sit_at_both_ends(self): + """A known-length reach's duplicate sits at the far end, not the middle. + + Its two edges (start->end breakpoint, end breakpoint->end node) are + both tagged boundary=True (0.0), and the connecting edge in between + carries the pipe's full real length. + """ + network = Network.from_epanet(_EPANET_RES, inp=_EPANET_INP) + pipe = network._reaches["10"] + + assert pipe.breakpoints[0].distance == 0.0 + assert pipe.breakpoints[1].distance == pytest.approx(3209.544) + + start_alias = network.find(reach="10", distance="start") + far_alias = network.find(reach="10", distance=3209.544) + end_alias = network.find(reach="10", distance="end") + assert len({start_alias, far_alias, end_alias}) == 3 + + def test_pump_breakpoint_stays_at_start_only(self): + """The pump's unknown length means only its leading breakpoint is real.""" + network = Network.from_epanet(_EPANET_RES, inp=_EPANET_INP) + pump = network._reaches[_PUMP_REACH] + + assert pump.breakpoints[0].distance == 0.0 + assert pump.breakpoints[1].distance is None def test_node_ids_overlapping_reach_ids_are_not_confused(self): """Most IDs here name both a node and a reach, e.g. '9', '10', '21'.""" @@ -1570,8 +1665,45 @@ def test_extra_node_quantities_are_merged(self): "WaterQuality", "Volume", "Volume Percentage", + "Flow", + "Velocity", + "HeadlossPer1000Unit", + "AvgWaterQuality", + "StatusCode", + "Setting", + "ReactorRate", + "FrictionFactor", + "Pump efficiency", + "Pump energy costs", + "Pump energy", } + def test_extra_reach_quantities_are_merged_onto_the_pump(self): + """resx's reach-level quantities merge onto the reach's breakpoints. + + Pump energy, efficiency, and cost, alongside its own Flow/Velocity/etc. + """ + network = Network.from_epanet(_EPANET_RES, resx=_EPANET_RESX) + pump = network._reaches[_PUMP_REACH] + + assert pump.breakpoints + for breakpoint in pump.breakpoints: + assert "Pump energy" in breakpoint.data.columns + assert "Flow" in breakpoint.data.columns + + def test_only_the_pump_reach_gains_resx_reach_quantities(self): + """The .resx covers only the pump reach, not the other twelve.""" + network = Network.from_epanet(_EPANET_RES, resx=_EPANET_RESX) + + non_pump_reaches = { + k: v for k, v in network._reaches.items() if k != _PUMP_REACH + } + assert non_pump_reaches + assert all(reach.breakpoints for reach in non_pump_reaches.values()) + for reach in non_pump_reaches.values(): + for breakpoint in reach.breakpoints: + assert "Pump energy" not in breakpoint.data.columns + def test_only_the_nodes_present_in_the_resx_gain_them(self): """The .resx covers the tank and the reservoir, not all eleven nodes.""" network = Network.from_epanet(_EPANET_RES, resx=_EPANET_RESX) @@ -1594,7 +1726,12 @@ def test_values_come_through(self): assert volume.notna().all() def test_selective_loading_still_governs_what_is_read(self): - network = Network.from_epanet(_EPANET_RES, resx=_EPANET_RESX, nodes=["2"]) + # reaches=[] isolates this to node selection: without it, every + # reach's breakpoints would also get real data now (reaches=None + # loads all of them), adding unexpected columns. + network = Network.from_epanet( + _EPANET_RES, resx=_EPANET_RESX, nodes=["2"], reaches=[] + ) df = network.to_dataframe() tank = network.find(node="2") From 42ec2fa886e4d17c3f5887c3999c38fc038ac5fb Mon Sep 17 00:00:00 2001 From: jpalm3r Date: Tue, 11 Aug 2026 10:36:56 +0200 Subject: [PATCH 2/2] Skip unknown-distance breakpoints when resolving an alias MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _resolve_alias walks the alias map comparing each key's distance against the requested one. A breakpoint with distance None — now possible on a link-node reach of unknown length — made that arithmetic raise a TypeError, even when a sibling breakpoint on the same reach would have resolved cleanly. Skip those keys instead: such a breakpoint is unreachable by distance, not missing. Co-Authored-By: Claude Opus 5 --- src/modelskill/model/network.py | 13 ++++++++++++- tests/test_network.py | 16 ++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/modelskill/model/network.py b/src/modelskill/model/network.py index 328c1cdea..f4d64b4f2 100644 --- a/src/modelskill/model/network.py +++ b/src/modelskill/model/network.py @@ -237,6 +237,13 @@ def _extract_reach(self, observation: ReachObservation) -> NodeModelResult: continue if item not in breakpoint.data.columns: continue + # A breakpoint with an unknown position can't be looked up by + # distance - find() treats distance=None the same as "not + # provided" and would raise. Such a breakpoint is unreachable + # this way, not missing; skip it rather than error, since + # another breakpoint on the same reach may still resolve. + if breakpoint.distance is None: + continue int_id = self.network.find( reach=breakpoint.id[0], distance=breakpoint.distance @@ -304,7 +311,11 @@ def _resolve_alias(self, alias: int | str | tuple[str, float]) -> int: reach_id, distance = alias candidates: list[tuple[float, int]] = [] for key, node_id in self.network._alias_map.items(): - if isinstance(key, tuple) and key[0] == reach_id: + if ( + isinstance(key, tuple) + and key[0] == reach_id + and key[1] is not None + ): diff = abs(key[1] - distance) if diff <= self._CHAINAGE_TOLERANCE: candidates.append((diff, node_id)) diff --git a/tests/test_network.py b/tests/test_network.py index 07e8962a7..2d115b022 100644 --- a/tests/test_network.py +++ b/tests/test_network.py @@ -1544,6 +1544,22 @@ def test_find_reach_end_breakpoint_with_unknown_length_is_not_addressable(self): with pytest.raises(KeyError): network.find(reach="10", distance=100.0) + def test_resolve_alias_tolerance_match_skips_unknown_distance_breakpoint(self): + """NetworkModelResult._resolve_alias must not crash on a None-distance sibling. + + Reach "10" has two breakpoints: one at distance 0.0, one at an + unknown distance (None). Resolving a nearby-but-not-exact numeric + distance walks the alias map's tolerance-matching loop, which must + skip the None-distance key rather than compute abs(None - distance). + """ + network = Network.from_epanet("./tests/testdata/epanet.res") + nmr = NetworkModelResult(network, item="Flow", name="epanet_model") + + exact_id = nmr._resolve_alias(("10", 0.0)) + nearby_id = nmr._resolve_alias(("10", 0.0005)) + + assert nearby_id == exact_id + def test_mike_file_is_redirected(self): with pytest.raises(ValueError, match=r"Use Network\.from_mike\(\)"): Network.from_epanet("./tests/testdata/network.res1d")