Promote reach-end gridpoints to breakpoints, remove boundary - #694
Draft
jpalm3r wants to merge 52 commits into
Draft
Promote reach-end gridpoints to breakpoints, remove boundary#694jpalm3r wants to merge 52 commits into
jpalm3r wants to merge 52 commits into
Conversation
Topology-only nodes store an empty DataFrame (not None), so the existing `is not None` check never dropped them. Concatenating an empty RangeIndex frame alongside real DatetimeIndex frames degraded the result index to object dtype, which later failed the "time must be datetime" assertion in ms.match(). Also skip empty frames so the DatetimeIndex is preserved. Fixes #676 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ComparerCollection.save()/load() doesn't support the NODE geometry
type: Comparer.load() has no "node" branch and falls into
NotImplementedError("Unknown gtype: node").
Refs gh #677
Comparer.save() only flattened raw_mod_data into the netcdf for gtype == "point", so node comparers silently dropped their raw model data on save. Extend the check to also cover "node". Part of gh #677
load() dispatched "track" / "vertical" / "point" and raised NotImplementedError for anything else, so a node comparer's netcdf could never be loaded back. Reconstruct NodeModelResult from the flattened _raw_* variables, mirroring the existing point branch, using the node coordinate carried along on each variable. Fixes gh #677
ms.match() raises a bare pandas TypeError ("Cannot compare tz-naive
and tz-aware datetime-like objects") when the observation and model
result disagree on timezone-awareness, with no ModelSkill-specific
context about what went wrong.
Refs gh #678
No timezone handling existed anywhere in modelskill, so a mismatch between a tz-aware and tz-naive time index only surfaced as a bare pandas TypeError deep inside observation.trim(), with no indication of what went wrong. Validate tz-awareness compatibility between the observation and each model result before trimming, and raise a ValueError naming both sides. Fixes gh #678
mypy inferred ts's type from its first assignment (NodeModelResult), flagging the later PointModelResult assignment as incompatible. Part of gh #677
…s earlier Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Two costs profiling exposed in from_res1d on large networks (gh #679): every topology-only location builds its own empty DataFrame, and a node shared by several reaches is read once per reach endpoint even though _generate_graph keeps only the first copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every topology-only node and gridpoint allocated its own empty frame: two per reach, 35% of a filtered load on the test file and ~5s on a reported 8,377-reach network. _build_dataframe already skips empty frames before the concat, so the shared instance never reaches pandas.
_load_res1d_network visited a node once for every reach it belongs to, but _generate_graph keeps only the first copy, so the repeat reads were thrown away: 236 reads for 119 nodes on the test file, 16,754 for 7,955 on a reported network. Boundary reads stay per-reach.
gh #679 asks for selective ingestion by quantity. Locations that do not carry a requested quantity should become topology-only rather than raise, and the filter should compose with the existing nodes/reaches filters.
Applied at the read layer, so the full topology is still built. Pushing the filter into the Res1D constructor instead is faster to open but drops every location that lacks the quantity, which loses the topology entirely on files where nodes and reaches hold different quantities. A location that carries none of the requested quantities becomes topology-only rather than raising. Closes #679
The tz-awareness guard added in 39c70f7 only covers a mismatch between an aware and a naive side; matching fails just as hard when both sides are aware, since the internal time coordinate cannot carry a timezone. Pin down the intended behaviour instead: convert tz-aware input to UTC and drop the timezone, so aware input works and two timezones pair up on absolute time. Replaces the test expecting a ValueError. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xarray cannot carry a timezone through operations such as interp or dropna, and TimeSeries.time drops it silently, so matching failed for any timezone-aware input - not only when one side was naive and the other aware. Convert aware time to UTC and drop the timezone where the time coordinate is normalised, which now happens for every geometry via _validate_dataset as well as for from_matched. Aware input works, two timezones pair up on absolute time, and a warning records the shift. This replaces the tz-awareness guard in _match_space_time, which could only reject the aware/naive combination and let differing timezones through to the same bare pandas TypeError. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Topology-only nodes store an empty DataFrame (not None), so the existing `is not None` check never dropped them. Concatenating an empty RangeIndex frame alongside real DatetimeIndex frames degraded the result index to object dtype, which later failed the "time must be datetime" assertion in ms.match(). Also skip empty frames so the DatetimeIndex is preserved. Fixes #676 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ComparerCollection.save()/load() doesn't support the NODE geometry
type: Comparer.load() has no "node" branch and falls into
NotImplementedError("Unknown gtype: node").
Refs gh #677
Comparer.save() only flattened raw_mod_data into the netcdf for gtype == "point", so node comparers silently dropped their raw model data on save. Extend the check to also cover "node". Part of gh #677
load() dispatched "track" / "vertical" / "point" and raised NotImplementedError for anything else, so a node comparer's netcdf could never be loaded back. Reconstruct NodeModelResult from the flattened _raw_* variables, mirroring the existing point branch, using the node coordinate carried along on each variable. Fixes gh #677
ms.match() raises a bare pandas TypeError ("Cannot compare tz-naive
and tz-aware datetime-like objects") when the observation and model
result disagree on timezone-awareness, with no ModelSkill-specific
context about what went wrong.
Refs gh #678
No timezone handling existed anywhere in modelskill, so a mismatch between a tz-aware and tz-naive time index only surfaced as a bare pandas TypeError deep inside observation.trim(), with no indication of what went wrong. Validate tz-awareness compatibility between the observation and each model result before trimming, and raise a ValueError naming both sides. Fixes gh #678
mypy inferred ts's type from its first assignment (NodeModelResult), flagging the later PointModelResult assignment as incompatible. Part of gh #677
…s earlier Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Two costs profiling exposed in from_res1d on large networks (gh #679): every topology-only location builds its own empty DataFrame, and a node shared by several reaches is read once per reach endpoint even though _generate_graph keeps only the first copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every topology-only node and gridpoint allocated its own empty frame: two per reach, 35% of a filtered load on the test file and ~5s on a reported 8,377-reach network. _build_dataframe already skips empty frames before the concat, so the shared instance never reaches pandas.
_load_res1d_network visited a node once for every reach it belongs to, but _generate_graph keeps only the first copy, so the repeat reads were thrown away: 236 reads for 119 nodes on the test file, 16,754 for 7,955 on a reported network. Boundary reads stay per-reach.
gh #679 asks for selective ingestion by quantity. Locations that do not carry a requested quantity should become topology-only rather than raise, and the filter should compose with the existing nodes/reaches filters.
Applied at the read layer, so the full topology is still built. Pushing the filter into the Res1D constructor instead is faster to open but drops every location that lacks the quantity, which loses the topology entirely on files where nodes and reaches hold different quantities. A location that carries none of the requested quantities becomes topology-only rather than raising. Closes #679
The tz-awareness guard added in 39c70f7 only covers a mismatch between an aware and a naive side; matching fails just as hard when both sides are aware, since the internal time coordinate cannot carry a timezone. Pin down the intended behaviour instead: convert tz-aware input to UTC and drop the timezone, so aware input works and two timezones pair up on absolute time. Replaces the test expecting a ValueError. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xarray cannot carry a timezone through operations such as interp or dropna, and TimeSeries.time drops it silently, so matching failed for any timezone-aware input - not only when one side was naive and the other aware. Convert aware time to UTC and drop the timezone where the time coordinate is normalised, which now happens for every geometry via _validate_dataset as well as for from_matched. Aware input works, two timezones pair up on absolute time, and a warning records the shift. This replaces the tz-awareness guard in _match_space_time, which could only reject the aware/naive combination and let differing timezones through to the same bare pandas TypeError. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
These two tests (originally 57e8611 on PR #685) called the removed Network.from_res1d, since #685 forked before #687 renamed it to from_mike/from_epanet. The rebase applied them without a merge conflict because the surrounding lines didn't overlap textually, but the calls were left broken. Rename the test functions too, matching the test_from_mike_* convention used elsewhere in this file.
…nto network-loading
9506fca rewrote _build_dataframe to surface reach boundary data via a 3-level column index, but crashed on any node without boundary data (pd.concat({}, axis=1) on the empty-boundary case), collided reach labels between a reach's two endpoints, and broke every downstream consumer of the 2-level column shape (to_dataframe(sel=...), to_dataset(), quantities). 25 failed, 29 errors on this branch. Reach quantities will be exposed by promoting reach-end gridpoints to ordinary breakpoints instead (a graph-construction change, not a dataframe-shape change), so this rewrite isn't needed to reach that goal.
Res1DReach now includes a reach's first/last gridpoint as ordinary breakpoints (not just the intermediate ones), so their data is reachable through the normal breakpoint mechanism instead of the removed boundary concept. This is what #599 and #680 actually need: reach-end quantities (e.g. EPANET Flow/Velocity, or MIKE's reach-end WaterLevel, which is a distinct series from the node's own) become ordinary graph data instead of being silently discarded. Endpoint gridpoint loading is now gated by populate_gridpoints like every other breakpoint, so reaches=[] suppresses it too - one consistent rule instead of a special case for what used to be boundary data. Since a reach-end breakpoint is geometrically coincident with its node, _generate_graph tags that edge boundary=True and clamps its length to an exact 0.0. The clamp matters because the edge's raw length is the difference between two independently-sourced values (mikeio1d's gridpoint chainage and the reach's total length), which are not guaranteed to be bit-identical - left unclamped, floating-point noise could produce a tiny negative weight and break shortest-path algorithms that require non-negative edges. `length` itself stays float | None on every edge, unchanged, so existing networkx weighted-algorithm behavior (None hides an edge from Dijkstra, per networkx's `cost is None` check) is untouched. Deliberately scoped to MIKE (res1d/res11): EPANET's reaches have at most 2 gridpoints, so the existing gate (len(reach.gridpoints) > 2) already excludes them, leaving today's EPANET behavior (no breakpoints, length=None) unchanged. EPANET's single synthetic gridpoint belongs to neither end and needs its own placement rule - that's #680's separate, still-open ask.
Reach-end data now lives on an ordinary breakpoint (previous commit), so nothing constructs or reads a node's boundary dict anymore. Drops the abstract property from NetworkNode, its param/property from BasicNode and Res1DNode, and the merge-on-revisit logic in _generate_graph. This is a public API break for any NetworkNode subclass: `boundary` was a required abstract property. Docs and the one notebook that referenced it are updated separately.
NetworkNode now requires two properties (id, data), not three - drop boundary from both example subclasses and the "required properties" callout. Rewrite the find/recall framing paragraph, which no longer introduces a separate boundary concept, and add a callout documenting the new zero-length boundary edges for anyone running weighted networkx algorithms on network.graph. res1d_network_mapping.png still shows boundary as a separate concept from find/recall; regenerating it is out of scope here.
Reword the two markdown cells describing graph nodes and the junction/breakpoint visualisation, now that boundary is no longer a graph-node attribute. The plot_network() widthmap used 'boundary' in g.nodes[node] to tell real topology nodes apart from break points; switch to the node's alias type instead (str for a real node, (reach, distance) tuple for a break point), which was already on every graph node and needs no new attribute. Notebook is excluded from automated execution (pythonnet/3.14), so this is a source-only fix; not re-rendered in this environment.
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
1 participant
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.
Important
Stacked on #685 (
network-loading) — merge that first. This diff will include #685's changes until then; the description below covers only what this branch adds on top ofnetwork-loading.Summary
Reach-end gridpoint data (e.g. EPANET's
Flow/Velocity, or MIKE's reach-endWaterLevel, which is a distinct series from the node's own) was previously held in aboundarydict onNetworkNodethat nothing ever read. This PR removesboundaryand instead promotes a MIKE reach's first/last gridpoint to an ordinary breakpoint, so that data is reachable throughfind()/recall()/to_dataframe()like any other break point.Res1DReachnow includes a reach's own start/end gridpoint in its breakpoints (previously only the intermediate ones), gated bypopulate_gridpointslike every other breakpoint —reaches=[]now consistently means "no gridpoint data at all," including these._generate_graphconnects them with a zero-length edge, taggedboundary=Trueand clamped to exactly0.0(the raw value is a difference between two independently-sourced numbers, so floating-point noise could otherwise leave a tiny negative weight and break shortest-path algorithms).NetworkNode.boundary(andBasicNode/Res1DNode'sboundaryparam) are removed — a breaking change for any existingNetworkNodesubclass._build_dataframe(25 failed / 29 errors on this branch beforehand) — it turned out not to be needed, since breakpoints already flow into the dataframe through the same mechanism as any other graph node.boundaryare updated accordingly.Deliberately out of scope: EPANET. Its reaches have at most one synthetic gridpoint belonging to neither end, so the existing gate (
len(reach.gridpoints) > 2) already excludes them — EPANET keeps today's behavior (no breakpoints,length=None) unchanged.Closes #599.
Related to #680, which stays open: this only reaches MIKE's reach-end data. EPANET's single-gridpoint case needs its own placement rule and is left for a follow-up.
Test plan
uv run pytest tests/— 814 passed, 6 skippeduv run ruff check/uv run mypyon all touched filesnetwork.res1d/network_cali.res11fixtures