Speed up from_* and add a quantities filter

[jpalm3r]

Speeds up Network.from_* and adds the quantity filter requested in #679.

Closes #679.

crcoDHI profiled a 7,955-node EPANET-backed file and found that the quantity filter — the literal
request — is the smallest of the available wins. Two cheaper fixes in the same code path matter
more, so all three are here.

Changes

• Share one empty DataFrame across topology-only locations (fac935e7). Every topology-only
  node and gridpoint allocated its own empty frame, two per reach: 35% of a filtered load on the
  test file and ~5 s on the reported network. _build_dataframe already skips empty frames before
  the concat, so the shared instance never reaches pandas.
• Read each selected node once (12bb3f2c). _init_node read a node once per reach endpoint,
  but _generate_graph keeps only the first copy. 236 reads for 119 nodes on the test file. The
  per-reach boundary read stays uncached — it is genuinely distinct.
• quantities parameter on from_* (22178858), applied at the read layer so the full
  topology is still built. None = all, str or list = subset, [] = none. Pushing the filter into
  the Res1D constructor instead is faster to open but drops every location lacking the quantity —
  crcoDHI confirmed zero of 8,377 reaches survive. A location carrying none of the requested
  quantities becomes topology-only rather than raising.

Regression tests land first in 57e86117 and d534e096; user guide in 2c663898.

On tests/testdata/network.res1d a filtered load goes from 176 ms to 132 ms under cProfile, with
378 empty-frame allocations down to 1 and 236 node reads down to 119. quantities=None reproduces
the previous behavior exactly.

Caveats

• The fixture carries exactly one quantity per location, so the tests verify the quantity filter is
  correct but never exercise a location holding two — the only case where reading per quantity
  saves anything. Asked crcoDHI to check against their file.
• The shared empty frame is a shared object. Nothing mutates a topology-only location's data
  today and a comment says not to, but a caller doing so would touch every such location at once.
  Allocating lazily per instance gives back only half the win.

Merge order

Requires #681 to close first. This branch is stacked on beta_test_found_bugs, so the diff
against main currently includes that PR's commits as well.

Not in this PR, filed from crcoDHI's profile: topology reuse (#682), batched reads (#683), and the
_get_total_length gap (#684).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR improves performance and flexibility of Res1D network ingestion by optimizing Network.from_res1d and introducing a quantities filter, while also adding/adjusting regression tests and user-guide documentation. It also includes stacked changes related to clearer timezone mismatch errors in matching and node-geometry save/load support.

Changes:

• Add quantities filtering to Network.from_res1d, applied at the per-location read layer while preserving full topology.
• Reduce ingestion overhead by sharing a single empty DataFrame for topology-only locations and caching per-node reads to avoid duplicate work.
• Expand regression tests and documentation; include stacked fixes for timezone-mismatch error clarity and node-gtype raw data save/load.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_network.py	Adds regression tests for datetime index integrity, empty-frame sharing, node read de-duplication, and quantities filtering behavior.
tests/test_match.py	Adds a regression test ensuring timezone-awareness mismatch raises a clear ValueError.
tests/test_comparercollection.py	Adds a node-geometry round-trip save/load test via ComparerCollection.
src/modelskill/network.py	Adds quantities parameter and threads it through Res1D loading; caches node reads; filters empty frames in dataframe build.
src/modelskill/model/adapters/_res1d.py	Implements quantity-filtered reads and introduces a shared _EMPTY_DATA to avoid repeated empty-frame allocations.
src/modelskill/matching.py	Adds _check_timezone_compatibility() to raise a clearer error before pandas/xarray failures.
src/modelskill/comparison/_comparison.py	Extends Comparer.save()/load() raw-data handling to include gtype == "node".
docs/user-guide/network.qmd	Documents the new quantities argument and provides an example.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/modelskill/model/adapters/_res1d.py:16

• Using a single module-level empty DataFrame shared by all topology-only nodes/gridpoints changes observable behavior: if any caller mutates network.graph.nodes[...]["data"] in-place, it will affect every topology-only location at once. If this is acceptable, it should be documented more explicitly as part of the public API; otherwise consider storing None for topology-only data in the graph (and only materializing an empty DataFrame on demand) to avoid both allocations and shared-mutation hazards.

# Topology-only nodes and gridpoints all share this frame instead of each
# allocating its own. A large network has two per reach, which profiling showed
# to be the biggest single cost of a filtered load. Never mutate it in place.
_EMPTY_DATA = pd.DataFrame()

[Copilot]

Pull request overview

Copilot reviewed 22 out of 27 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/modelskill/model/adapters/_res1d.py:28

• _simplify_colnames() returns a new pd.DataFrame() when node.quantities is empty. These locations are effectively topology-only too (e.g. MIKE 11 nodes), so this defeats the shared empty-frame optimization described just above (_EMPTY_DATA) and can reintroduce many empty-frame allocations on formats with quantity-less locations.

    # Some formats keep no timeseries at all on some locations - MIKE 11, for instance,
    # stores everything on reach gridpoints, leaving the nodes empty. Asking mikeio1d
    # for a dataframe there raises, so return an empty one instead.
    if not node.quantities:
        return pd.DataFrame()

[jpalm3r]

Ran a cProfile comparison of Network.from_epanet on main vs. this branch, loading a network of a similar scale to the one referenced above (roughly 8,000 nodes, ~8,400 reaches; can't share the file itself).

	main	network-loading
Wall time	~64s	~36s
Function calls	57.9M	50.9M
Node data reads	33,508	24,709

About a 1.8x speedup, and the drop in node reads lines up with the per-reach-endpoint caching fix.

[ecomodeller]

Scope note: this review covers only the timezone commit (d61f083). I haven't reviewed the network loading work itself yet — the request for changes is about the commit riding along, not the substance of the PR.

The timezone commit belongs in its own PR

d61f0835 ("Convert timezone-aware time to UTC-naive on construction") is unrelated to network loading — nothing in this PR produces timezone-aware time, and the tz tests exercise the generic point and from_matched paths. It's also a user-visible behaviour change: input that previously raised now silently succeeds with shifted data. Please revert it here and take it separately.

Three objections, in case they help shape the follow-up.

1. Two concerns under one name. _normalize_time_to_ns was a mechanical pandas 3.0 resolution shim with no semantics. Renaming it to _normalize_time and folding in timezone handling merges a compat workaround with a domain decision. If both survive, they belong in separate functions called in sequence.

2. The warning isn't a safeguard. In practice nobody reads warnings, so a warnings.warn next to a silent time shift ships the shift while feeling covered. Either the behaviour is acceptable with no notice at all, or it should raise.

3. The conversion asserts something the user never said. ModelSkill deliberately has no notion of timezones, as it has none of CRS — that is the user's to track, and it's a single line of pandas as preprocessing. A timezone-aware timestamp is unambiguous, so re-expressing it as UTC is lossless and fine in principle. The mixed case is not: converting an aware observation to UTC to pair it with a naive model asserts that the naive side is UTC, about data the user never annotated. That is the one combination that cannot be read unambiguously — and exactly the one the deleted _check_timezone_compatibility rejected.

On the deleted guard

The commit message argues the guard "could only reject the aware/naive combination and let differing timezones through to the same bare pandas TypeError". That's a fair complaint about the implementation, not about the rule.

Worth correcting one implied premise, though: the guard was not dead code. xarray does retain a timezone-aware coordinate —

>>> ds.time.dtype
dtype('<M8[ns, Europe/Copenhagen]')
>>> ds.time.to_index().tz
<DstTzInfo 'Europe/Copenhagen' LMT+0:50:00 STD>

— so _check_timezone_compatibility read the timezone correctly and fired as intended.

What is true is that interp rejects any tz-aware coordinate, not just mismatched ones:

TypeError: Cannot interpret 'datetime64[ns, Europe/Copenhagen]' as a data type

So supporting two aware series in different timezones does require converting to UTC-naive somewhere. Construction is the wrong place, because it cannot see the other side — which is precisely why this commit had to swallow the mixed case along with the rest. The principled home is match time: check compatibility first, then convert the all-aware case together.

Suggested rule

Be generous with input that can be read unambiguously, strict about output. Concretely: accept aware, accept naive, accept two different timezones; raise on aware mixed with naive, with a message naming the one-line pandas fix. No conversion at construction, no warning.

That's a design change worth its own review, so: revert here, and the rest of the PR isn't blocked on settling it.

[jpalm3r]

Measured against a large real network

Loaded a 41 MB EPANET .res holding 7,955 nodes and 8,377 reaches, comparing main, this branch, and this branch with quantities="Pressure". Both revisions ran in one virtual environment with PYTHONPATH pointed at a worktree of main, so mikeio1d and networkx stayed on the same builds and only modelskill's own code differed.

Work per load

Wall-clock on the test machine varied up to 4x for byte-identical work, so the headline figures are cProfile call counts, which are exactly reproducible.

revision	total calls	vs main	_simplify_colnames	to_dataframe	DataFrame.__init__
main	57,969,971	1.00x	33,508	33,508	1,269,572
this branch	50,941,132	1.14x	24,709	24,709	1,067,195
this branch, quantities="Pressure"	35,956,009	1.61x	24,709	7,955	448,985

The counts match what the changes claim to do, down to the last call:

• 8,377 reaches × 2 endpoints = 16,754 node visits against 7,955 unique nodes, so caching node data saves exactly 8,799 calls. 33,508 − 8,799 = 24,709, which is what we get. That is 1c02135.
• With the filter, to_dataframe drops to 7,955 — one per node, none for the link gridpoints, which carry no Pressure and return the shared empty frame without reading anything. That is fac935e and 2217885 working together.
• DataFrame construction falls 65% against main.

Passing an .inp adds a flat 663,829 calls in every variant, which is the [PIPES] parse and is unaffected by any of this.

Wall-clock

From the least noisy run, minimum of three loads: 24.2 s on main, 20.1 s on this branch, 14.2 s filtered — so 1.21x and 1.70x. Those agree with the call counts closely enough to quote roughly 1.15x unfiltered and 1.6–1.7x with the filter, but the machine was too noisy for a firmer number.

One thing to know about .resx

The three-file case could not be measured. The .resx available for this model holds four nodes that do not exist in the sibling .res, and only 7 of its 11 nodes match, so _open_companion_result rejects the pair. That guard behaves the same on main, so it is a property of those two files rather than anything this branch changed. The numbers above therefore cover .res alone and .res plus .inp.

Harness

tests/profiling/compare_network_loading.py runs this comparison, driving profile_network_loading.py once per case in a fresh process. Point --baseline-src at another checkout to reproduce it.