Skip to content

Per-product Network constructors, with EPANET companion files - #687

Merged
jpalm3r merged 24 commits into
mainfrom
read-network-res
Aug 4, 2026
Merged

Per-product Network constructors, with EPANET companion files#687
jpalm3r merged 24 commits into
mainfrom
read-network-res

Conversation

@jpalm3r

@jpalm3r jpalm3r commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Why

Network.from_res1d read far more than res1d files. mikeio1d's single Res1D class opens nine extensions across five products, so the name promised one format and delivered nine. Rather than guess which of the nine work, I ran every one of mikeio1d's own fixtures through the loader — which turned up more than a naming problem.

One constructor per product

Only where a committed fixture backs it:

Network.from_mike(res)                              # .res1d, .res11   MIKE 1D, MIKE 11
Network.from_epanet(res, resx=None, inp=None)       # .res + companions

Both delegate to one private _from_mikeio1d, so adding a product later is a docstring and one call. Passing a file the other one reads raises ValueError: ... Use Network.from_epanet() instead.

from_res1d is removed rather than deprecated: it only shipped in the 1.4.0a3 alpha, and the network module is opt-in and absent from the API reference, so a shim would have added a second name for the tested path without protecting a real caller.

EPANET companion files

An EPANET run writes more than one file, and the .res is not the whole picture:

File What it adds
.res The network and its main timeseries. Required.
.resx Extra results — tank volume, pump energy. Merged onto matching nodes.
.inp The model input. The only one of the three carrying reach lengths.
network = Network.from_epanet("model.res", resx="model.resx", inp="model.inp")

inp= gives 12 of the 13 reaches real lengths (3209.5, 1609.3 m …); the pump keeps None, since [PIPES] is the one section with lengths. resx= adds Volume and Volume Percentage on the tank and reservoir.

.resx was previously refused as if it were a broken network. It is not a network at all — its node and reach IDs are a strict subset of the sibling .res, carrying different quantities — so it belongs as an argument, not a constructor. A companion is validated against the main file (same time axis, no unknown IDs) rather than merged on trust, since two runs would line up silently and produce a network nothing downstream would flag.

mikeio1d does not read .inp at all, so model/adapters/_inp.py parses the sections we need. No new dependency: pulling in wntr or swmmio for two sections each would weigh more than the parser (ADR-010). It is shared with SWMM, which uses the same .inp layout.

Not covered: resx= merges node quantities only. Its reach-level quantities (pump energy, efficiency, costs) sit on single-gridpoint reaches with no breakpoint to live on — #680.

Reach length is now optional

Reach length matters in rivers and sewer networks and not in link-node models, so NetworkReach.length is no longer an @abstractmethod. It defaults to None, subclasses override it only where a length exists, and BasicReach's argument defaults to None too.

The adapter also stops trusting mikeio1d's 0. ResultReach.length returns 0 when the length cannot be read — its own docstring says so — which every EPANET reach hits without an .inp. Reporting that as a zero-length reach makes a length-weighted graph algorithm treat the pipe as free, so it maps to None and networkx fails loudly instead: shortest-path treats the edge as unreachable, weight-summing calls raise TypeError. Dropping the edge attribute would have been worse, since networkx defaults a missing weight to 1.

Two bugs found on the way

MIKE 11 never worked. _simplify_colnames called to_dataframe() on nodes with no quantities — MIKE 11 keeps its timeseries on reach gridpoints, so its nodes are empty — and mikeio1d raised Could not create DataFrame with zero items. Guarding on quantities fixes it: network_cali.res11 now loads with 3 reaches, 71 nodes, real reach lengths and 23/21/23 breakpoints.

.resx failed opaquely. Res1DReach.__init__ compared start_node.id != reach.start_node with None on both sides, so the check passed and networkx then complained about a None node key three layers from the cause. An explicit is None guard names it instead.

Also added: a TypeError for a Res1D opened with a Path, which otherwise dies with AttributeError: 'WindowsPath' object has no attribute 'endswith' from inside mikeio1d, on every format.

Formats still refused, and why

Extension Reason
.out (SWMM) The connectivity is not in the .outStartNodeIndex is -1 on every reach, coordinates are nan, no chainages. It lives in the companion .inp, which we do not read yet. Tracked in #689 with the pairing verified and one blocker written up.
.prf, .crf, .xrf (MOUSE), .whr (Water Hammer) No result file exists in this repo or in mikeio1d's testdata, so support cannot be verified. Tracked in #686.
.resx Not refused any more in the sense that matters — read it via from_epanet(res, resx=...). Passing it as the primary file still raises, and the message says so.

Each refusal now names the file that would lift it. The old wording blamed mikeio1d for not exposing reach start/end nodes, which was true of our code path but wrong about the cause and left the user nowhere to go. (#688 was opened on that mistaken reading and is closed, with a comment explaining what it got wrong.)

Fixtures

Five files copied unchanged from DHI/mikeio1d (MIT, same licence as modelskill) at commit d937466, with provenance in tests/testdata/README.md: network_cali.res11, epanet.res, epanet.resx, epanet.inp, swmm.out. About 261 KB total. swmm.out is deliberately kept without its .inp, so the refusal test fails the day SWMM support lands.

Tests

One is worth pointing at: the two readable extension sets plus the refusal table must union to exactly Res1D.get_supported_file_extensions(). A mikeio1d release that adds a tenth format fails CI and forces a decision, rather than leaving the format silently unreachable.

Otherwise: TestEpanetCompanionInp and TestEpanetCompanionResx cover the lengths, the merge, selective loading, and every refusal path — the two mismatched-file guards need monkeypatching, since the committed fixtures are a matching pair by construction. TestReadInp covers the parser. TestOptionalReachLength covers a subclass that omits length, the None edge attribute, break point distances surviving an undefined total, and both loud networkx failures.

Verification

  • full suite: 793 passed, 6 skipped
  • ruff check src, mypy src/ and the metrics doctests clean
  • new and existing user-guide cells execute from docs/user-guide
  • checked against a large confidential EPANET result (~8000 reaches) that cannot be committed

Notes for review

  • just docs was not run end to end; I executed the {python} cells directly instead.
  • Network still has no quartodoc API page. It had none before, so these constructors are documented in the user guide and their docstrings only. Happy to add one if you want it in the API reference.
  • The first three commits on the branch predate the design discussion: they widen from_res1d and test it, and later commits remove it. I left the history as it happened rather than rewriting. a5d1faee has a misleading subject — it only adds a .gitignore rule, it does not commit any data.
  • Node20 has been updated to Node24 in the full_test CI pipeline.

Design decisions are recorded in adr/012-network-format-constructors.md.

jpalm3r and others added 12 commits August 3, 2026 10:06
Res1D reads nine formats (res1d, res11, res, prf, crf, xrf, out, whr,
resx), so the hardcoded .res1d-only guard rejected files that load fine.
Ask Res1D for the supported set rather than keeping a second copy of it
that drifts as mikeio1d adds formats.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The extension guard and the TypeError branch had no coverage.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MIKE 11 stores its timeseries on reach gridpoints, so its nodes carry no
quantities at all and mikeio1d raises "Could not create DataFrame with
zero items" when asked for one. Guard on quantities instead, which
unblocks .res11 files.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mikeio1d returns None for both start_node and end_node on .resx results,
which the existing identity checks let through by comparing None to None.
The graph build then failed with networkx complaining about a None node
key, three layers from the cause. Check explicitly instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four files copied unchanged from DHI/mikeio1d (MIT, same as modelskill):
res11 and epanet.res to cover MIKE 11 and EPANET end to end, resx and
swmm.out to assert that the two formats mikeio1d cannot give us reach
connectivity for are rejected with a clear message.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Moves the version guard, extension check, Res1D construction and
nodes/reaches normalisation into _from_mikeio1d, so the per-format
constructors added next are each a docstring and one delegating call.
No behaviour change: from_res1d passes allowed=None.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four of the nine extensions mikeio1d reads cannot produce a Network.
SWMM .out and .resx expose no reach start/end nodes, so there is no
topology to rebuild; MOUSE and Water Hammer have no test fixture
anywhere, so support cannot be verified. Each now fails with the
specific reason instead of an error from inside mikeio1d.

The rejection tests use real .resx and .out files, so they start failing
if a future mikeio1d exposes connectivity for them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mikeio1d resolves reach topology with str.endswith on Res1D.file_path,
so a Res1D built from a Path raises AttributeError from three frames
down, blaming an attribute the caller never touched. Say what is wrong
up front instead. Re-opening it ourselves would discard whatever filters
the caller set on their object.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Name the constructors after the products that write the files, so the
method list is the format list. Each is a docstring and one delegating
call; passing a file the other one reads raises a ValueError naming it.

from_epanet documents the link-node caveats, and the tests assert them:
zero-length edges, no breakpoints, and ReachObservation therefore not
being matchable against an EPANET network.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The name promised one format while the method read nine, which is what
prompted the split. Callers move to from_mike, which reads exactly the
.res1d files the old name referred to. Removed outright rather than
deprecated: it only ever shipped in the 1.4.0a3 alpha, and the network
module is opt-in and absent from the API reference.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The user guide claimed Res1D was the only supported format. Replace that
with the constructor table, the reasons the other mikeio1d formats are
refused, runnable MIKE 11 and EPANET examples, and a callout for the
EPANET link-node caveats. ADR-012 records the naming decision and the
rule that a constructor requires a fixture.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jpalm3r
jpalm3r requested a review from ecomodeller as a code owner August 3, 2026 11:10
Copilot AI review requested due to automatic review settings August 3, 2026 11:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the opt-in Network loader API to be product-specific (MIKE vs EPANET) rather than format-name-specific, adds explicit extension policy/guardrails around what modelskill will and won’t load from mikeio1d, and fixes two loader/adapter edge cases discovered while validating across mikeio1d fixtures.

Changes:

  • Replaces Network.from_res1d with Network.from_mike and adds Network.from_epanet, both delegating to a shared _from_mikeio1d implementation with centralized extension validation.
  • Fixes MIKE 11 empty-node handling in the Res1D adapter and improves failure clarity when reach connectivity is absent (e.g., .resx).
  • Adds/updates tests, docs, ADR, and test fixtures/provenance to lock in the supported/refused format policy.

Reviewed changes

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

Show a summary per file
File Description
src/modelskill/network.py Introduces from_mike/from_epanet, shared loader, and extension validation/policy tables.
src/modelskill/model/adapters/_res1d.py Guards _simplify_colnames for locations with no quantities; raises clearer error when reach nodes are missing.
src/modelskill/model/network.py Updates user-facing error text to reference from_mike.
tests/test_network.py Updates existing tests to from_mike, adds extension policy and adapter unit tests, adds product-specific constructor tests.
tests/testdata/README.md Documents provenance for newly added third-party fixtures.
tests/notebooks/test_notebooks.py Updates skip-list comment to reflect from_mike name.
docs/user-guide/network.qmd Updates user guide to new constructors and documents/refuses unsupported formats.
notebooks/Collection_systems_network.ipynb Updates notebook examples to from_mike.
adr/012-network-format-constructors.md Records the design decision and rationale for per-product constructors.
adr/README.md Adds ADR-012 to the index.
roadmap/features/network-models.md Updates roadmap claims/status to include MIKE 11 + EPANET and notes unsupported formats.
.gitignore Ignores confidential testdata directory.

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

Comment thread src/modelskill/network.py
Copilot AI review requested due to automatic review settings August 3, 2026 11:19
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Copilot AI review requested due to automatic review settings August 3, 2026 11:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

jpalm3r added 4 commits August 3, 2026 13:56
Reach length matters in river and sewer networks but not in link-node water
distribution models, where no length exists to supply. Drop the abstractmethod
so subclasses may omit it, default BasicReach's argument to None, and guard the
one graph edge that needs the total length.
mikeio1d returns 0 when it cannot read a reach length, which every EPANET reach
hits. Surfacing that as a zero-length reach makes length-weighted graph
algorithms treat the reach as free; None makes them raise instead.
Covers a NetworkReach subclass that omits length, the None edge attribute it
produces, break point distances surviving an undefined total, and the mikeio1d
zero sentinel becoming None.
Records why an unreadable length is surfaced as None rather than 0, and why the
edge attribute is kept rather than omitted.
Copilot AI review requested due to automatic review settings August 3, 2026 12:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

jpalm3r added 6 commits August 3, 2026 15:23
mikeio1d reads only the binary result formats, so the companion input file has
to be parsed here. Both products share one layout, so the section reader is
generic and only the [PIPES] interpretation is EPANET-specific.
Copied from mikeio1d at the same commit as the other vendored fixtures. Carries
the pipe lengths the .res file does not.
An EPANET run writes the network and main timeseries to .res, extra results to
.resx, and the model itself to .inp - which is the only one of the three that
carries reach lengths. Accept both companions as keyword arguments.

resx= merges node quantities only. Its reach-level quantities sit on
single-gridpoint reaches with no breakpoint to live on (#680).

Planned as two commits, but the signature and loader plumbing are shared, so
splitting would have left a half-built argument list in between.
The old message blamed mikeio1d for not exposing reach start/end nodes. The real
reason is that neither file carries its own topology: SWMM's lives in the
companion .inp, and .resx describes a network defined in its sibling .res. The
.resx message now names the argument that reads it.
Covers real pipe lengths from inp=, the pump keeping None, merged node
quantities from resx=, both together, and each refusal path. The two guards
against mismatched files need monkeypatching, since the committed fixtures are
a matching pair by construction.
Adds the companion-file table and a worked example, corrects the refused-format
table, and records two supporting rules: companions are constructor arguments
rather than constructors, and a refusal names the file that would lift it.
Copilot AI review requested due to automatic review settings August 3, 2026 13:44
@jpalm3r jpalm3r changed the title Replace Network.from_res1d with per-product constructors Per-product Network constructors, with EPANET companion files Aug 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (3)

.github/workflows/full_test.yml:61

  • Same version-pin inconsistency here: actions/checkout@v6 differs from the @v4 used in the repo’s other workflows. Aligning pins helps keep CI behavior predictable.
      - uses: actions/checkout@v6

.github/workflows/full_test.yml:27

  • Same as earlier: this job uses actions/checkout@v6 while other workflows pin @v4 (see .github/workflows/docs.yml:23). Consider aligning versions unless there’s a specific need for v6.
      - uses: actions/checkout@v6

.github/workflows/full_test.yml:13

  • This workflow pins actions/checkout@v6, but the rest of the repo’s workflows still use actions/checkout@v4 (e.g. .github/workflows/docs.yml:23). Unless v6 is intentionally required, keeping the same major version across workflows reduces maintenance and avoids unexpected CI breakage if v6 isn’t available in all environments.

This issue also appears in the following locations of the same file:

  • line 27
  • line 61
      - uses: actions/checkout@v6

@jpalm3r
jpalm3r merged commit 8651801 into main Aug 4, 2026
13 checks passed
@jpalm3r jpalm3r mentioned this pull request Aug 4, 2026
jpalm3r added a commit that referenced this pull request Aug 4, 2026
jpalm3r added a commit that referenced this pull request Aug 4, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants