Per-product Network constructors, with EPANET companion files (replica of #687) - #691
Merged
Conversation
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>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors modelskill.network.Network file-based construction to be product-specific (MIKE vs EPANET), adds support for EPANET companion files (.resx for extra node quantities and .inp for pipe lengths), and updates docs/tests/fixtures accordingly to make format support explicit and well-tested.
Changes:
- Replace the ambiguous
Network.from_res1dentry point withNetwork.from_mikeand addNetwork.from_epanet, both delegating to a shared private loader with explicit extension validation and clearer refusal messages. - Add an
.inpparser (_inp.py) to read EPANET pipe lengths and merge.resxnode quantities into the main.resnetwork. - Make
NetworkReach.lengthoptional (Nonewhen undefined) and update graph construction, documentation, fixtures, and tests to reflect the new behavior and supported/refused formats.
Reviewed changes
Copilot reviewed 14 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/modelskill/network.py |
Adds per-product constructors, extension policy tables, companion handling, and optional reach-length behavior in graph construction. |
src/modelskill/model/adapters/_res1d.py |
Fixes MIKE 11 empty-node handling, adds .resx node-quantity merge helper, and makes reach length optional/overrideable. |
src/modelskill/model/adapters/_inp.py |
Introduces a minimal EPANET/SWMM-style .inp section parser and EPANET pipe-length reader. |
src/modelskill/model/network.py |
Updates user-facing guidance/error message to reference Network.from_mike(...) instead of from_res1d(...). |
tests/test_network.py |
Updates tests to new constructors and adds coverage for extension policy, optional reach length, EPANET companions, and adapter behavior. |
tests/notebooks/test_notebooks.py |
Updates notebook skip comment to reference the renamed constructor. |
docs/user-guide/network.qmd |
Documents the per-product constructors, refused formats, EPANET companions, and implications of undefined reach lengths/breakpoints. |
notebooks/Collection_systems_network.ipynb |
Updates example usage to Network.from_mike(...). |
adr/README.md |
Adds ADR-012 to the ADR index. |
adr/012-network-format-constructors.md |
New ADR documenting the rationale for per-product constructors and explicit refusal policy. |
roadmap/features/network-models.md |
Updates roadmap text to reflect MIKE 11 + EPANET support and format limitations. |
tests/testdata/README.md |
New provenance documentation for vendored mikeio1d fixtures. |
tests/testdata/network_cali.res11 |
Adds a MIKE 11 fixture to cover .res11 network reading. |
tests/testdata/epanet.res |
Adds an EPANET .res fixture for from_epanet coverage. |
tests/testdata/epanet.resx |
Adds an EPANET .resx fixture for companion-quantity merge coverage. |
tests/testdata/epanet.inp |
Adds an EPANET .inp fixture for pipe-length coverage. |
tests/testdata/swmm.out |
Adds a SWMM fixture to pin refusal behavior until topology support lands. |
.gitignore |
Ignores tests/testdata/confidential/* and normalizes uv.lock entry formatting. |
.github/workflows/full_test.yml |
Updates workflow checkout action version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ecomodeller
approved these changes
Aug 4, 2026
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
Replica of #687, which was merged and then reverted (#690) before it should have landed — see #690 for why. #687's own branch (
read-network-res) can't be reopened as a PR: it's already an ancestor ofmain's history from the original merge, so GitHub sees zero commits to compare.This branch is a fresh cherry-pick of #687's 23 commits onto current
main, verified to introduce exactly the same 19-file changeset #687 did (git diff --stat origin/main network-constructors-v2), with no unrelated drift —mainpicked up an unrelatedkeep_duplicatesremoval (#657) after #687 branched, and this replica correctly leaves that alone rather than reintroducing it.Original description, unchanged:
---## Why
Network.from_res1dread far more than res1d files. mikeio1d's singleRes1Dclass 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:
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 raisesValueError: ... Use Network.from_epanet() instead.from_res1dis 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
.resis not the whole picture:.res.resx.inpinp=gives 12 of the 13 reaches real lengths (3209.5, 1609.3 m …); the pump keepsNone, since[PIPES]is the one section with lengths.resx=addsVolumeandVolume Percentageon the tank and reservoir..resxwas 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
.inpat all, somodel/adapters/_inp.pyparses the sections we need. No new dependency: pulling inwntrorswmmiofor two sections each would weigh more than the parser (ADR-010). It is shared with SWMM, which uses the same.inplayout.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.lengthis no longer an@abstractmethod. It defaults toNone, subclasses override it only where a length exists, andBasicReach's argument defaults toNonetoo.The adapter also stops trusting mikeio1d's
0.ResultReach.lengthreturns0when 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 toNoneandnetworkxfails loudly instead: shortest-path treats the edge as unreachable, weight-summing calls raiseTypeError. Dropping the edge attribute would have been worse, sincenetworkxdefaults a missing weight to1.Two bugs found on the way
MIKE 11 never worked.
_simplify_colnamescalledto_dataframe()on nodes with no quantities — MIKE 11 keeps its timeseries on reach gridpoints, so its nodes are empty — and mikeio1d raisedCould not create DataFrame with zero items. Guarding onquantitiesfixes it:network_cali.res11now loads with 3 reaches, 71 nodes, real reach lengths and 23/21/23 breakpoints..resxfailed opaquely.Res1DReach.__init__comparedstart_node.id != reach.start_nodewithNoneon both sides, so the check passed and networkx then complained about aNonenode key three layers from the cause. An explicitis Noneguard names it instead.Also added: a
TypeErrorfor aRes1Dopened with aPath, which otherwise dies withAttributeError: 'WindowsPath' object has no attribute 'endswith'from inside mikeio1d, on every format.Formats still refused, and why
.out(SWMM).out—StartNodeIndexis-1on every reach, coordinates arenan, 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).resxfrom_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 intests/testdata/README.md:network_cali.res11,epanet.res,epanet.resx,epanet.inp,swmm.out. About 261 KB total.swmm.outis 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:
TestEpanetCompanionInpandTestEpanetCompanionResxcover 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.TestReadInpcovers the parser.TestOptionalReachLengthcovers a subclass that omitslength, theNoneedge attribute, break point distances surviving an undefined total, and both loudnetworkxfailures.Verification
ruff check src,mypy src/and the metrics doctests cleandocs/user-guideNotes for review
just docswas not run end to end; I executed the{python}cells directly instead.Networkstill 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.from_res1dand test it, and later commits remove it. I left the history as it happened rather than rewriting.a5d1faeehas a misleading subject — it only adds a.gitignorerule, it does not commit any data.Design decisions are recorded in
adr/012-network-format-constructors.md.