Build the network model result on mikeio1d's topology layer - #702
Draft
jpalm3r wants to merge 45 commits into
Draft
Build the network model result on mikeio1d's topology layer#702jpalm3r wants to merge 45 commits into
jpalm3r wants to merge 45 commits into
Conversation
Reopens the question ecomodeller raised on #694, with the seam set where NetworkModelResult already draws it: modelskill keeps the comparer-facing classes, mikeio1d gets the formats, the graph and the Network class itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 1 landed as mikeio1d #247, so the decision is no longer a proposal: the module is built upstream and the Phase 0 snapshots pass against it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A network result file identifies nodes and reaches by ID, but observations are usually recorded against real-world station names held in the MIKE+ setup database. Read that sqlite database to resolve a station to the node or reach it sits on, so observations can be placed without hand-mapping every ID. The resolver is one class in obs.py rather than a module of its own: everything MIKE+ specific -- the table names, the join, the locationtype codes, the encoding of resitemname -- sits in one class body, and its only caller is a few lines below it. It returns a list of _Station rather than a DataFrame with five agreed column names, so the contract is in the type. Dataset variables also gain a long_name attribute, so a quantity keeps its label once it reaches xarray. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A node observation records whatever the caller addressed it with, so a comparer
built from at="node_A" already carries the name. The model side disagreed: it
recorded the integer the network handed out, and load() coerced the coordinate
back with int(), which raises on a name. A breakpoint was worse off again -- it
carries reach and distance rather than node, and several places tested for
"node" alone.
- One list of the coordinates that say where a timeseries sits, so the three
places that dropped a subset of them on the way to a dataframe now drop the
same set. Fixes NodeObservation(df, at=("r1", 24.5)).to_dataframe(), which
raised because the node coordinate it dropped is not there.
- NodeModelResult takes a node name or a (reach, distance) pair, and records the
graph integer beside it as node_index. Neither identity nor load reads that
integer back.
- load() stops re-deriving the location: the coordinates travelled with the
file. A reach comparer can now be saved and loaded at all, where it used to
raise NotImplementedError.
The backend is unchanged, so extraction still hands out integers; the next
commit swaps it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The extra is renamed networks -> network, matching mikeio1d's own, so modelskill[network] and mikeio1d[network] read the same. It shipped in the 1.4.0a3 alpha only, so nothing needs a deprecation. networkx and xarray now arrive through mikeio1d's extra rather than being named here. The module is unreleased, so a uv source points at mikeio1d's main branch. Both entries carry a TODO to swap it for a version floor once mikeio1d releases; per ADR-013, modelskill 1.4.0 waits for that release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
modelskill's own topology layer goes: the abstract types, the Res1D adapter, the per-product constructors, the EPANET companions, the extension policy table and the .inp reader. All of it now lives in mikeio1d, where the formats and the fixtures already were (ADR-013). 1503 lines of source, and the CI failure that came with the extension table -- a mikeio1d release adding a format is no longer our problem. NetworkModelResult takes a path or a mikeio1d Network. A path goes to Network.open, which picks the reader and finds the EPANET companions; refusals for .out, .resx and the fixture-less formats are upstream's to word now. The network is used as given rather than deep-copied, which was the largest cost of building a model result on a river model. Extraction reads the dataset and consults the topology only to explain a failure. Two consequences worth knowing: - A location whose column is entirely NaN now counts as having no data. It used to be selected, and produced a match with no points, so the observation quietly disappeared; another break point on the reach is used instead. - Break points with an unknown distance now take part. That is EPANET read without its .inp, where no reach has a length and the second break point of every reach is unaddressable by distance. Node lookups delegate to Network.find, so there is one chainage tolerance rather than a second copy here, and a failed lookup names the near misses instead of listing the first five aliases in the map. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A network hands out integers to label its graph, and which integer a location gets depends on the network it was built from and the mikeio1d that built it. Nothing a user writes should depend on that, so at= now takes a node name or a (reach, distance) pair, and an integer raises with the recipe for getting the name back. The signature shipped in the 1.4.0a3 alpha only, so it goes without a shim. The guard catches np.int64 as well as int: Comparer.node returns a numpy scalar, and isinstance(np.int64(5), int) is False. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The user guide loses the sections that documented the topology layer -- building a network, the companions, selective loading, inspecting the graph, find and recall -- and gains a short section saying where the network comes from and how to hand modelskill a path. What stays is the part modelskill still owns: the skill assessment workflow and the MIKE+ database lookup. 587 lines to 197. The notebook addressed its observations by graph integer and highlighted junctions by reading a node attribute that upstream moved to edges; both are fixed, and it runs end to end. It is in the notebook SKIP_LIST, so CI would not have caught either. ADR-012 is narrowed rather than superseded: the constructors and the extension tables it argues for are mikeio1d's now, and the reasoning still applies there. ADR-010's open question about version constraints is answered for this feature. The res1d mapping diagram went with the section that explained it. The fixtures for the formats whose tests moved are kept, and the testdata README now says nothing here reads them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The alpha wrote the graph integer into the node coordinate. Nothing on the load path derives a location from it any more, so such a file still loads and skills -- but a regression there would be silent, so the fixture is a file the alpha actually wrote rather than one built to look like it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ADR-013 was written as a proposal and parts of it no longer describe what was built: mikeio1d collapsed from_mike and from_epanet into Network.open, the extra is called network, and it named a fixture this repository does not have. The snapshot paragraphs said the same thing twice, once in each tense. ADR-012's note described its own position in the document. It now sits under the status line and says what moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Inverted constructions, closing kickers and sentences that restate the one before them. Stacked clauses split into separate sentences. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…nto network-phase-2
A reach-gtype comparer fell through the gtype dispatch to NotImplementedError, even though the node branch's body is already correct for it: _drop_scalar_coords drops reach and distance too.
A breakpoint that exists but holds nothing for the selected quantity surfaced as "All datetime indices must be non-empty" from matching, because every timestep was dropped as NaN. The reach path already treats an all-NaN column as no data; the node path now does too.
The example paired breakpoint 21.285 on 94l1 with a WaterLevel model result, but that breakpoint only carries Discharge, so the block could not run -- and its plain python fence kept quarto from ever executing it. Point it at 42.57, which does carry WaterLevel, and make the fence executable so the render catches this next time.
The source filter was an unanchored LIKE '%name%' on tsfilename, so it could not tell calib.dfs0 from my_calib.dfs0 -- it still reported the item as ambiguous and told the caller to pass the source they had just passed -- and an underscore in a real file name acted as a wildcard. Compare file names in pandas instead, splitting on the Windows separator the database stores.
Every row matching the requested item names was classified before any quantity or kind filter, so a station whose locationtype is not a place in the network -- a rain gauge or a catchment registered in the same file -- failed the whole resolve, and on_missing="skip" could not help. Such stations are now left out, and named in the error only when they were what the caller asked for.
Its name was fed to the station resolver as a database item filter, so supplying metadata failed unless the name happened to match the database's own -- which the docstring never said it had to. A string selects; a Quantity describes.
jpalm3r
marked this pull request as ready for review
September 3, 2026 12:44
_coords.py is a module of coordinate types. location_from_coords was the one piece of logic in it, and what it returns is NodeObservation.at's type -- a node name or a (reach, distance) pair. Both callers already import obs.py, so the move costs no new import edge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
at and _create_new_instance each hand-wrote the node/breakpoint branch that _location_from_coords already performs. _at_from_coords wraps it with the coercion the at argument is declared with, so the as-recorded and coerced forms sit next to each other instead of being implicit in two copies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Which coordinates make a dataset network data was written out by hand in five places, in four different spellings. They still agreed, so this changes no behaviour -- it stops the next edit from splitting them. GeometryType.from_coords is the one home every caller can reach: types.py is the leaf that timeseries, comparison, obs and model all import, and the enum it returns is defined there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TimeSeries._coordinate_values, Comparer._coordinate_values and _scalar were three copies of the same four lines. Both methods and _scalar now delegate to one module-level function; _scalar keeps its .item() unwrap on top. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TimeSeries carried a node property and a __repr__ branch that named node coordinates. The repr branch was dead: it was gated on gtype == "point", but any dataset with a node coordinate is given gtype "node", so a node observation printed no location at all. __repr__ now asks an overridable _location_repr(), which NodeObservation, ReachObservation and NodeModelResult answer with where they sit -- so they start reporting a location they never showed before. node moves to NodeObservation, the only class it ever said anything about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_point.py, _track.py and _vertical.py each own their parse function; network was the only geometry without one, so its two parsers sat in _point.py. The edge is one way -- _network -> _point -> _coords -- and timeseries/__init__ re-exports the same names, so no caller changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GeometryType.from_coords read only network coordinates, but sat on a generic enum under a name that promised to classify any dataset. A plain x/y point got None rather than POINT, which the name gave no warning of. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_coordinate_values sat in _timeseries.py while _coords.py held everything else about coordinates. Moving it also lets _coords.py read coordinates without importing _timeseries.py, which already imports _coords.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_location_from_coords lived in obs.py yet served model and comparison too, and it reads the same coordinates NodeCoords and ReachCoords write. It moves beside them as network_location. _at_from_coords stays in obs.py, since coercing to NodeObservation's `at` types is its own affair. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The network tests reached past the objects a user holds and asserted on what backs them: the xarray coordinate layout instead of NodeObservation.at and .gtype, the private _create_new_instance instead of trim(), the unexported NodeModelResult, and NetworkModelResult.data. One test assigned to NetworkModelResult.data to reach a guard no caller can trigger; another replaced Network.to_dataset to fake a unit, re-testing Quantity.from_cf_attrs, which test_quantity.py already covers. Import the public names from modelskill, so the suite also proves they are exported, and move the ReachBreakPoint stub and network builder shared with the other test modules into tests/network_helpers.py. Adds the public surface that had no test at all: ms.observation() routing at= and reach= to the right class, Comparer.distance, NodeObservation.node on a breakpoint, and ReachObservation's weight and attrs. Drops two fixtures left over from the pre-mikeio1d xr.Dataset API. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two network fixtures deep-imported NetworkModelResult and the observations, and carried a second copy of the ReachBreakPoint stub that tests/network_helpers.py now holds. The 1.4.0a3 test called the private _to_observation to see an integer refused, which TestNodeObservationAliases already covers on the constructor itself. Adds the skill() and plot() calls a node or reach comparer had no test for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ms.match() with a ReachObservation was only ever reached through a comparer fixture, so the reach path had no test of its own in test_match.py. Add one, and put the third copy of the hand-built network next to the other two in tests/network_helpers.py, which now also builds the one-reach network whose data sits on a break point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GeometryType.from_network_coords is the one place the rule lives, and its doctests were never collected: the doctest target named metrics.py alone. Also takes the MIKE+ observations from modelskill rather than modelskill.obs, and drops the integer node keys two of its tests still pass, which NodeObservation no longer accepts as a location. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The res1d values are checked against mikeio1d's own read of the file, which does not go through the Network layer the model result is built on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
The MIKE+ station lookup is coming out of this PR and onto its own branch: #706. It is unrelated to the mikeio1d move (ADR-013, Phase 2), and its schema assumptions — |
It published the integers to_dataset() hands out, which ADR-013 says users should not handle: the numbering belongs to one network built by one mikeio1d version, which is why at=<int> went and node_index is provenance nothing reads back. It was also the only member of this class that could not be answered without building the dense (time, node) x quantity rectangle. Ryan's review asks for a lazy source later (mikeio1d #250), and everything else here already allows it: matching uses only name and extract(obs). Removing this now keeps the eager rectangle an implementation choice rather than something 1.4.0 promises. Nothing in src/ or the docs read it. Topology questions belong to mr.network, which is mikeio1d's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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.
Phase 2 of the move recorded in ADR-013. mikeio1d now owns the layer that reads
network result files and builds the graph; modelskill keeps the model result, the
observations, the matching and the MIKE+ lookup, and consumes what mikeio1d
produces.
Phase 1 was mikeio1d #247, where the
module was built and checked against snapshots of this loader's output.
What goes
src/modelskill/network.py(1205 lines),model/adapters/_res1d.py(189) and_inp.py(109), with the tests that covered them. The extension table goes too,and with it the test that failed our CI whenever a mikeio1d release added a
format.
What changes
NetworkModelResulttakes a path or amikeio1d.network.Network. A path goes toNetwork.open, which picks the reader and finds the EPANET companion files.Observations are addressed by the name the model uses, or by
(reach, distance)for a break point.
at=<int>is gone: which integer a location gets depends onthe network and the mikeio1d that built it, so nothing a user writes should
depend on it. That signature shipped in the 1.4.0a3 alpha only.
Extraction reads the dataset and consults the topology only to explain a failure.
Node lookups go through
Network.find, so there is one chainage tolerance ratherthan a second copy here, and a failed lookup names the near misses.
The extra is renamed
networks→network, matching mikeio1d's.Bugs fixed along the way
Each has a test that fails on
main.Comparer.loadcoerced the node coordinate withint(), so a comparer builtfrom
at="node_A"could not be reloaded.NotImplementedError: Unknown gtype: reachon load.NodeObservation(df, at=("r1", 24.5)).to_dataframe()raised, because it droppeda
nodecoordinate a break point does not have.Behaviour changes
reach. It used to be selected and produce a match with no points, so the
observation quietly disappeared. Another break point on the reach is used
instead.
EPANET read without its
.inp, where no reach has a length.mr.networkis the caller's object, not a deep copy.Does this block a lazy source?
No. Logged as mikeio1d #250.
matching.pyusesm.nameandm.extract(obs)on aNetworkModelResultand nothing else,so a lazy source replaces the constructor and
extractand leaves the rest of this PR alone.atalready takes a node name or(reach, distance). The location travels into the matcheddataset as scalar coordinates taken from the network, with
node_indexbeside them asprovenance nothing reads back, so a source that never mints integers omits it.
NetworkModelResult.nodesis removed here. It published the integersto_dataset()handsout, which ADR-013 says users should not handle, and it was the only member that could not be
answered without building the rectangle. Nothing in
src/or the docs read it..data,.timeand.sel_itemsstay;DfsuModelResult.datais already a file handle, so theypromise nothing about materialisation.
One cost does not go away: a
ReachObservationreads every break point series on its reach,to check they agree before picking one.
Draft: two things block merging
[tool.uv.sources]points at itsmain branch. Both
networkentries carry a TODO to swap that for a versionfloor. ADR-013 holds modelskill 1.4.0 until mikeio1d releases.
deletes, and their code already shipped in mikeio1d Plots directional axis #247. Locate network observations with a MIKE+ database #698 (MIKE+ lookup)
and Build a NetworkModelResult straight from a result file #699 (path constructor) are carried here — Locate network observations with a MIKE+ database #698 cherry-picked, Build a NetworkModelResult straight from a result file #699
rewritten onto
Network.open.Checks
769 passed, 5 skipped.
ruff check src,ruff format --check,mypyand themetrics doctests are clean. The user guide renders. The notebook runs end to end,
which CI does not check — it is in
SKIP_LIST.tests/regression/test_regression_rose.pyfails, onmainas well: the lockfilepins matplotlib 3.10.9 and the baseline image was made with a newer one.
Notes for review
workflow and the MIKE+ section.
_mikeplus.pywas folded intoobs.pyas one class,_MikePlusStationResolver,next to its only caller. Its 23 tests were rewritten to go through
NodeObservation.from_multiple(db=...).tests/testdata/node_comparer_1.4.0a3.ncis a file the alpha actually wrote. Itpins that such a comparer still loads.
except
network.res1d.🤖 Generated with Claude Code