Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands network-based workflows by introducing edge-based observations and alias/breakpoint resolution, enabling extraction/matching at arbitrary breakpoints along edges (not only named nodes). It also adds selective loading options for large Res1D networks and updates the user guide accordingly.
Changes:
- Added
EdgeObservationand extendedNetworkModelResult.extract()to support both node- and edge-based extraction, including alias resolution for string IDs and(edge, distance)breakpoints. - Implemented selective loading in
Network.from_res1d(nodes=..., reaches=...)to reduce memory usage while preserving full topology. - Updated documentation and tests for alias handling and selective loading behavior.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_network.py | Adds tests for selective loading and alias/breakpoint resolution. |
| tests/test_match.py | Updates error-message expectation for network matching. |
| src/modelskill/timeseries/_timeseries.py | Extends dataset validation to accept edge / edge+distance coords. |
| src/modelskill/timeseries/_point.py | Adds EdgeCoords / breakpoint parsing and adjusts gtype attribution. |
| src/modelskill/timeseries/_coords.py | Introduces NodeCoords and new EdgeCoords. |
| src/modelskill/timeseries/init.py | Exposes _parse_network_breakpoint_input. |
| src/modelskill/obs.py | Adds EdgeObservation, gtype="edge", and node/breakpoint alias docs. |
| src/modelskill/network.py | Adds selective loading for Res1D, safer __repr__, and a copy() helper. |
| src/modelskill/model/network.py | Extends NetworkModelResult to extract by node alias/breakpoint and by edge. |
| src/modelskill/model/adapters/_res1d.py | Refactors Res1D adapters to support optional data loading for nodes/gridpoints. |
| src/modelskill/matching.py | Allows matching EdgeObservation to extracted NodeModelResult. |
| src/modelskill/comparison/_comparison.py | Tightens typing for raw_mod_data. |
| src/modelskill/comparison/_comparer_plotter.py | Adds mypy return-value suppression for populated axes list. |
| src/modelskill/comparison/_collection_plotter.py | Adds mypy return-value suppression for populated axes list. |
| src/modelskill/init.py | Re-exports EdgeObservation. |
| docs/user-guide/network.qmd | Documents selective loading, aliases, breakpoint usage, and edge observations. |
| docs/_quarto.yml | Adds EdgeObservation to docs navigation and quartodoc sections. |
| docs/.gitignore | Ignores generated *.quarto_ipynb notebooks. |
Comments suppressed due to low confidence (1)
src/modelskill/model/network.py:203
_extract_node()resolves string/tuple aliases viaself.network._alias_mapbut does not verify that the resolvednode_idis actually present inself.data.node(which can happen with selective loading). In that caseself.data.sel(node=...)will raise an unhelpfulKeyError. Consider validating membership after_resolve_alias()and raising aValueErrorexplaining that the node exists in the topology but its timeseries was not loaded (and how to load it viaNetwork.from_res1d(nodes=..., reaches=...)).
def _extract_node(self, observation: NodeObservation) -> NodeModelResult:
if observation.at is None and observation.node is None:
raise ValueError("NodeObservation must have either 'node' or 'at' set")
raw_id = observation.at if observation.at is not None else observation.node
assert raw_id is not None # Redundant assertion, included for mypy
node_id = self._resolve_alias(raw_id)
return NodeModelResult(
data=self.data.sel(node=node_id).drop_vars("node"),
node=node_id,
name=self.name,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/DHI/modelskill/sessions/27bd2a41-305c-4ba0-a543-f0f064cb98a6 Co-authored-by: jpalm3r <28826351+jpalm3r@users.noreply.github.com>
Agent-Logs-Url: https://github.com/DHI/modelskill/sessions/27bd2a41-305c-4ba0-a543-f0f064cb98a6 Co-authored-by: jpalm3r <28826351+jpalm3r@users.noreply.github.com>
Agent-Logs-Url: https://github.com/DHI/modelskill/sessions/3034caca-12b5-4d02-8b3e-8183de2f2c15 Co-authored-by: jpalm3r <28826351+jpalm3r@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Network data is stored in nodes or breakpoints and, to access the latter, you need the edge name and the distance within the edge (i.e. chainage). There are variables that are constant in all breakpoints from the same edge (e.g. discharge), thus requiring the chainage is redundant. In addition, in some domains, users typically do not use the chainage at all which creates friction.
This PR introduces the following changes:
Enhanced observation and matching capabilities:
EdgeObservationtype and integrated it throughout the codebase, allowing extraction and matching of model results at arbitrary breakpoints along network edges, not just at named nodes. [1] [2] [3] [4] [5] [6]NetworkModelResult.extract()method to support bothNodeObservationandEdgeObservation, with improved error messages and alias resolution for node and edge identifiers. [1] [2]Documentation improvements:
Example use