Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds initial support for network-based model results and observations in modelskill, aiming to enable matching/comparison workflows for node-indexed time series (e.g., 1D network models) alongside existing point/track workflows.
Changes:
- Introduces
NetworkModelResult/NodeModelResultandNodeObservation/NetworkObservationtypes and wires them intomatch()andComparertype annotations. - Updates
TimeSeriesvalidation and dataframe/export helpers to allow node-based coordinates (nox/y). - Adds a demonstration notebook for network workflows.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_timeseries.py | Updates validation error-message assertions for new coordinate rules. |
| src/modelskill/types.py | Adds GeometryType.NETWORK and a NetworkType alias. |
| src/modelskill/timeseries/_timeseries.py | Allows datasets with node coordinate instead of x/y; updates x/y accessors and to_dataframe() behavior. |
| src/modelskill/timeseries/_point.py | Adds network-node parsing and extends coordinate handling to include network coords. |
| src/modelskill/timeseries/_network.py | New network parsing helper (currently has multiple runtime issues). |
| src/modelskill/timeseries/_coords.py | Adds NetworkCoords. |
| src/modelskill/timeseries/init.py | Exposes new parsing helpers. |
| src/modelskill/obs.py | Adds NodeObservation and NetworkObservation; extends observation() factory logic. |
| src/modelskill/model/network.py | Adds NetworkModelResult and NodeModelResult with node extraction and alignment logic. |
| src/modelskill/model/init.py | Exports the new model result classes and updates docs. |
| src/modelskill/matching.py | Expands NetworkObservation into NodeObservations and supports NetworkModelResult extraction. |
| src/modelskill/comparison/_comparison.py | Broadens raw model type hints (but several point-only assumptions remain). |
| src/modelskill/init.py | Exposes network model/observation types at package root (incomplete for NodeModelResult). |
| notebooks/Collection_systems_res1d.ipynb | Demonstrates network matching and skill computation workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…st coordinate logic
…te validation in _include_coords
…ove NaN dropping in _parse_network_input
…treamline scalar coordinate dropping
10 tasks
…cessary conditional check
Adding explicit type checks before extraction. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… unused parameters and warnings
…bal_start_end function
…prove node handling
Introducing MultiNodeObservation class
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Bypassing merge rules since these are minimal changes to fix mypy issues
…Network1D protocol, update NetworkModelResult to accept Network object
…orkEdge from Protocol to ABC and add abstract methods
…tion; update tests to utilize new classes
Dict-only interface retained. The list-based overload is preserved at git tag archive/from-multiple-list-signature for future reference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Generalize types to create NetworkModelResults
| @abstractmethod | ||
| def id(self) -> str: | ||
| """Unique string identifier for this node.""" | ||
| ... |
| @abstractmethod | ||
| def data(self) -> pd.DataFrame: | ||
| """Time-indexed DataFrame with one column per quantity.""" | ||
| ... |
| @abstractmethod | ||
| def boundary(self) -> dict[str, Any]: | ||
| """Boundary-condition metadata dict (may be empty).""" | ||
| ... |
| @abstractmethod | ||
| def id(self) -> tuple[str, float]: | ||
| """``(edge_id, distance)`` tuple uniquely identifying this break point.""" | ||
| ... |
| @abstractmethod | ||
| def data(self) -> pd.DataFrame: | ||
| """Time-indexed DataFrame with one column per quantity.""" | ||
| ... |
| @abstractmethod | ||
| def id(self) -> str: | ||
| """Unique string identifier for this edge.""" | ||
| ... |
| @abstractmethod | ||
| def start(self) -> NetworkNode: | ||
| """Start (upstream) node of this edge.""" | ||
| ... |
| @abstractmethod | ||
| def end(self) -> NetworkNode: | ||
| """End (downstream) node of this edge.""" | ||
| ... |
| @abstractmethod | ||
| def length(self) -> float: | ||
| """Total length of this edge in network units.""" | ||
| ... |
| @abstractmethod | ||
| def breakpoints(self) -> list[EdgeBreakPoint]: | ||
| """Ordered list of intermediate :class:`EdgeBreakPoint` objects (may be empty).""" | ||
| ... |
…es1DReach and Network classes
Mypy issues
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.
This pull request extends the modelskill package to support network-based model results and observations. The main focus is on integrating
NetworkModelResultandNodeModelResulttypes throughout the matching, comparison, and data handling logic, enabling seamless workflows for network geometries alongside existing point, track, and field types.Key changes include:
Support for Network Model Results and Observations
NetworkModelResultandNodeModelResultto the public API and imports in__init__.pyandmodel/__init__.py, and updated documentation to reflect these new types. [1] [2] [3]NodeModelResultandNetworkModelResultalongside existing model result types. [1] [2] [3] [4] [5] [6] [7] [8]Matching and Extraction Logic
matchfunction and related helpers to support matching network observations and models, including correct extraction and alignment logic for network types. [1] [2] [3]Data Handling and Conversion
_drop_scalar_coordsutility to consistently remove scalar coordinates (includingnode) from xarray datasets when converting to pandas DataFrames, ensuring clean dataframes for all geometry types. [1] [2] [3] [4] [5]nodecoordinate andNodeObservation. [1] [2] [3] [4]Attribute and Geometry Type Handling
gtype) inference and handling throughout the codebase to support the new network type, ensuring correct behavior and error handling for all supported geometries. [1] [2]API and Type Annotation Consistency
These changes collectively enable robust support for network-based model results and observations, aligning the handling of network geometries with existing modelskill workflows.