Conversation
…nhance output formatting
…tion in the notebook
… for improved flexibility
…access to create_res1d_mapper
There was a problem hiding this comment.
Pull request overview
This PR refactors the Res1D network mapping implementation by introducing protocol-based abstractions and a factory function to create a mapper that can produce a generic network representation.
Changes:
- Added a new
_network_protocol.pymodule definingProtocols plusNetworkMapperandGenericNetwork. - Reworked
_network_mapper.pyto build protocol-conforming Res1D node/edge adapters and exposecreate_res1d_mapper(). - Updated tests and the network-mapping notebook to use
create_res1d_mapper()instead of constructingNetworkMapperdirectly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_network_mapper.py | Updates tests to construct the mapper via create_res1d_mapper() |
| notebooks/res1d_network_mapping.ipynb | Updates notebook import/usage to the new factory and refreshes outputs |
| mikeio1d/experimental/_network_protocol.py | New protocol + generic network + mapper implementation |
| mikeio1d/experimental/_network_mapper.py | Refactors Res1D-specific mapping to adapters + factory (create_res1d_mapper) |
| mikeio1d/experimental/init.py | Exposes create_res1d_mapper from the experimental package |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Check if all ids exist in the network | ||
| _CHAINAGE_TOLERANCE = 1e-3 | ||
|
|
||
| def _resolve_id(id): | ||
| if id in self._alias_map: | ||
| return self._alias_map[id] | ||
| if isinstance(id, tuple): | ||
| edge_id, distance = id | ||
| for key, val in self._alias_map.items(): | ||
| if isinstance(key, tuple) and key[0] == edge_id and abs(key[1] - distance) <= _CHAINAGE_TOLERANCE: | ||
| return val | ||
| return None | ||
|
|
||
| resolved = [_resolve_id(id) for id in ids] | ||
| missing_ids = [ids[i] for i, v in enumerate(resolved) if v is None] | ||
| if missing_ids: | ||
| raise KeyError( | ||
| f"Node/breakpoint(s) {missing_ids} not found in the network. Available nodes are {set(self._alias_map.keys())}" | ||
| ) |
There was a problem hiding this comment.
find() relies on self._alias_map being populated by a prior map_network() call; if find() is called first, it will always raise a KeyError with an empty/irrelevant "Available nodes" set. Consider guarding early with a dedicated error (e.g., ValueError telling the caller to run map_network() first) or lazily initializing the alias map.
| raise KeyError( | ||
| f"Node/breakpoint(s) {missing_ids} not found in the network. Available nodes are {set(self._alias_map.keys())}" |
There was a problem hiding this comment.
The KeyError message includes set(self._alias_map.keys()), which can be extremely large for realistic networks and may bloat logs / slow exception formatting. Consider truncating the list (e.g., show count + first N examples) or providing a separate method to inspect available IDs.
| raise KeyError( | |
| f"Node/breakpoint(s) {missing_ids} not found in the network. Available nodes are {set(self._alias_map.keys())}" | |
| total_available = len(self._alias_map) | |
| max_examples = 10 | |
| sample_ids: list[Any] = [] | |
| for key in self._alias_map.keys(): | |
| sample_ids.append(key) | |
| if len(sample_ids) >= max_examples: | |
| break | |
| raise KeyError( | |
| "Node/breakpoint(s) {missing} not found in the network. " | |
| "Available IDs (showing {shown} of {total}): {examples}".format( | |
| missing=missing_ids, | |
| shown=len(sample_ids), | |
| total=total_available, | |
| examples=sample_ids, | |
| ) |
…rtions with ValueError exceptions for incorrect node IDs
…r, updating related tests
… in code and tests
ryan-kipawa
left a comment
ryan-kipawa
left a comment
There was a problem hiding this comment.
Nice. I agree that the network protocols could live elsewhere, and I think they fit best in modelskill, but it's okay for them to live here until that happens. I didn't do a very thorough review, but it seems straight forward enough - the only important point is that I think protocols especially need good docstrings.
|
I have moved the implementation of protocols to this PR, in the |
|
After we close the open PR in modelskill, we might one to use this one to remove network support from |
This PR introduces a collection of different protocols to generate a network that can be mapped to a generic network structure. Also, it uses these new protocols to rewrite existing logic specific to Res1D objects.
We might want to place the
_network_protocolmodule someplace else (e.g. modelskill).In addition, we simplify user api to create generic
Networkobjects.