refactor: split the builder from the run layer in Catchment/Run/Calibration - #217
Merged
Merged
Conversation
Each script derived its YAML path from __file__, which lets it run from any working directory but obscures which file it loads at the call site. Written as the literal filename instead: coello-lumped-model-run.py now loads "coello-lumped-model-run.yaml" directly. Trade-off: this only resolves when the process's working directory is the script's own folder -- python coello-lumped-model-run.py from elsewhere now raises FileNotFoundError, where it previously worked from anywhere. Confirmed each script still runs correctly from its own directory.
The literal filename from the previous commit only resolved when the process's working directory was the script's own folder. Written as a repo-root-relative path instead -- run each script from the repo root, e.g. python examples/hydrological-model/coello/run/coello-lumped-model-run.py. Each docstring now says so explicitly. Confirmed all four scripts run correctly from the repo root and fail, as expected, from their own directory.
…butes A run used to leave its output as nine separate attributes on the Catchment it was handed, with a private `_maxbas_routed` boolean recording which routing scheme had written them. Two problems followed. A catchment's state was unknowable between runs, since a half-finished run and a finished one look alike and the routed fields of a previous run survive into the next. And the interpretation of the arrays sat on the input object rather than on the arrays, so three separate methods had to set and clear the flag by hand -- with a comment on the clearing explaining that a previous MAXBAS run may have left it set. SimulationResults holds them together, with the routing scheme as a field. The run layer builds one per run and assigns it to `Catchment.results`; the seven result arrays plus `qout` become read-only properties forwarding to it, so `Run.RunHapi(model); model.Qtot` reads exactly as before. `_maxbas_routed` becomes a property derived from `results.routing`, so it cannot outlive the run that set it. Read-only on purpose: these are outputs, and a run that can be half-overwritten by hand is what the results object exists to prevent. To stage a post-run state, build a SimulationResults and assign `model.results`. `extract_discharge` still fills `qout` on the Muskingum path, now through the results object. It cannot move into the engine: finding the outlet needs the gauge table, which is an analysis input rather than a run input.
`Run` subclassed `Catchment` so that a catchment could be passed where `self` was expected: every entry point was called unbound, as `Run.RunHapi(model)`. The inheritance was never a real IS-A -- `Run` added no state, never called `super().__init__`, and nothing in the codebase instantiated it or checked `isinstance`. But the inheritance was not the coupling that mattered. Measured across the layer, a run reached into 26 named attributes of the object it was handed and wrote 9 back. Declaring the parameter as `Catchment` would have removed the lie about `self` while leaving that unchanged. So the contract is stated instead. `hapi.protocols` declares what each kind of run requires -- ConceptualModelInputs, DistributedModel, LumpedModelInputs, FloodModel -- and `Catchment` satisfies them structurally, inheriting nothing. The dependency inverts: neither `hapi.run` nor `hapi.wrapper` imports `hapi.catchment` at runtime any more, which is asserted in a subprocess rather than assumed. The protocols live in their own module because `hapi.run` imports `hapi.wrapper` and both need them. The entry points return the `SimulationResults` they produced, threaded up from `DistributedRRM.run_lumped_model`, so no layer reads a result back off the model and re-narrows it from `| None`. `hapi.run` is off the mypy suppression list and the whole package type-checks clean. Three checks the protocols exposed as unguarded, each of which used to fail on None inside the validation and now names what is missing: a cell-to-cell run without a flow-direction raster, a lake entry point without a lake record, and the flood model without its river geometry. The flow-direction check also had three different wordings across the entry points for one identical test; they are now one constant, keeping the widest and most accurate of them. BREAKING CHANGE: `Run` is no longer a subclass of `Catchment`. `Run()` and `isinstance(x, Run)` no longer work, and the 14 public `Catchment` methods that were reachable through the inheritance -- `Run.save_results(model)` and the like -- are gone. Call them on the catchment. Every documented call form (`Run.RunHapi(model)`, `Run.runLumped(model, route, fn)`) is unchanged.
The previous two commits kept old call sites working on purpose: result attributes stayed readable off the catchment through forwarding properties, and the CamelCase entry points were left alone because renaming them would break downstream users. Neither concession is wanted -- this is a redesign, so where the new design says something different, the old thing goes. Results have one home. The eight forwarding properties on `Catchment` (`quz`, `qlz`, `state_variables`, `quz_routed`, `qlz_translated`, `q_total`, `qout`) and the derived `_maxbas_routed` are deleted; the arrays are read as `model.results.q_total`. `Qtot` becomes `q_total`, since new code follows the naming convention like everything else. `Run.from_yaml` is deleted too: it existed to intercept a call the old inheritance made resolvable, and with the inheritance gone the absent attribute is the honest answer. Every legacy CamelCase entry point is renamed: Run.RunHapi -> Run.run_distributed Run.runFW1 -> Run.run_maxbas Run.runHAPIwithLake -> Run.run_distributed_with_lake Run.RunFW1withLake -> Run.run_maxbas_with_lake Run.RunFloodModel -> Run.run_flood Run.runLumped -> Run.run_lumped Wrapper.RRMModel -> Wrapper.run_muskingum Wrapper.RRMWithlake -> Wrapper.run_muskingum_with_lake Wrapper.FW1 -> Wrapper.run_maxbas Wrapper.FW1Withlake -> Wrapper.run_maxbas_with_lake Wrapper.Lumped -> Wrapper.run_lumped DistributedRRM.SpatialRouting -> route_muskingum DistributedRRM.DistMaxbas1 -> route_maxbas DistributedRRM.DistMaxbas2 -> route_maxbas_by_path_length Calibration.FW1Calibration -> Calibration.calibrate_maxbas Calibration.lumpedCalibration -> Calibration.calibrate_lumped `DistributedRRM.Dist_HBV2` is deleted rather than renamed: it is a self-contained legacy reimplementation of run_lumped_model plus route_muskingum that nothing calls. `extract_discharge` loses `frame_work_1` and `only_outlet`. The first asked the caller to restate which entry point they had just called, and raised when they got it wrong; the routing is a property of the arrays, so it is read off `results.routing` instead and the right hydrograph is selected automatically. The second was documented as having no effect at all. Both are gone, and with them the ValueError that existed only to catch a mis-set flag. Examples, notebooks, docs and README are updated throughout. BREAKING CHANGE: no aliases and no deprecation period. Result arrays move from `model.<field>` to `model.results.<field>`, `Qtot` is `q_total`, every entry point listed above is renamed, `Run.from_yaml` and `DistributedRRM.Dist_HBV2` are removed, and `extract_discharge` no longer accepts `frame_work_1` or `only_outlet`.
… fields `start`, `end`, `temporal_resolution`, `date_index`, `dt` and `conversion_factor` described one thing. Three of them were inputs; the other three were computed from those in the constructor and then stored alongside, as if independent. Storing a derivation is how they drift: reassigning `end` left `date_index` describing the old span with nothing to notice. It is also why the same `pd.date_range` branch appeared four times across the package. `SimulationPeriod` holds the three inputs and derives the rest on read, so they cannot disagree, and it is frozen so a run covers the span it was built for. It validates the resolution and rejects a backwards span, which previously produced an empty `date_index` that failed much later as a zero-length driver mismatch naming neither date. This is not a typing exercise. The point is that a run should need a handful of coherent things from the model rather than a list of loose fields, and each of them should be constructed-or-absent rather than nullable -- the shape `MeteoInputs` and `FlowNetwork` already established here. The run layer's read surface drops from 22 named attributes to 17, and the four `date_range` copies become one derivation. `Lake` keeps its own `start` / `end` / `Index`: it is a separate object with its own record, not a catchment. BREAKING CHANGE: `Catchment.start`, `.end`, `.date_index`, `.dt`, `.conversion_factor` and `.temporal_resolution` are gone. Read them off `model.period` -- `model.period.date_index`, `model.period.conversion_factor`. `Catchment(...)` still takes the same constructor arguments.
…rce their rule Seven attributes described the conceptual model configured to run -- `parameters`, `snow`, `maxbas`, `lumped_model`, `area`, `initial_cond`, `q_init` -- set by two readers that did not know about each other. The cost was not untidiness. `snow` and `maxbas` *determine how many parameters there must be* (PARAMETER_COUNTS), and that rule was enforced in exactly one place: inside `read_parameters`, on the one assignment a calibration never makes. `calibration.py` replaced the array in six places, three of them inside the optimiser loop -- once per trial vector, thousands of times per run -- and none was checked. A distribution function producing the wrong width reached the per-cell loop and failed there as an index error, far from the call that caused it. Three objects now, one per reader, so no half-built state has to exist: ParameterSet(values, snow, maxbas) <- read_parameters ConceptualModelSetup(model, area, ...) <- read_lumped_model ParameterBounds(lower, upper, snow, maxbas) <- read_parameters_bound A first attempt paired the first two into one spec with a staging step. That was wrong: the readers are independent and either may run first, so pairing them forced exactly the half-built object this is removing, and it broke tests that exercise one reader alone. Splitting on the reader boundary removed the staging entirely. `ParameterSet` is frozen, and the calibration loops go through `with_values`, so every trial vector is checked as it arrives. `ParameterBounds` carries the `(snow, maxbas)` pair too, because a calibration reads no parameter file -- the bounds are where the configuration enters. The optimiser's answer moves to `Calibration.best_parameters`. It was assigned to `parameters`, but for a distributed calibration `res[1]` is the flat vector the search ran over, not the `(rows, cols, n)` array a run reads -- two shapes describing different things under one name, which left the model unrunnable after a calibration. mypy caught this the moment `parameters` became a typed object. The run layer's read surface drops from 17 named attributes to 12, and the protocol conflicts against `Catchment` are now only `| None` on assembled objects -- the builder-versus-finished-model question -- with nothing smeared left. BREAKING CHANGE: `Catchment.parameters` is a `ParameterSet`, not an array -- read `model.parameters.values`. `snow` and `maxbas` are on it. `lumped_model`, `area`, `initial_cond` and `q_init` move to `Catchment.model_setup`. `LB`/`UB` become `Catchment.bounds.lower`/`.upper`. `Calibration.parameters` no longer holds the optimiser result; use `best_parameters`.
… the loop
`route_muskingum` decided whether to skip a cell with
if Model.routing_method != "Muskingum" and Model.bankfull_depth[x, y] > 0:
which conflated two things. The intent is the flood model's: river cells are
routed by the kinematic-wave model, so the Muskingum pass leaves them alone. But
the comparison ran inside the routing loop on *every* distributed run, so a
catchment declaring `routing_method="Kinematic"` and calling `Run.run_distributed`
dereferenced `bankfull_depth` -- None outside the flood model -- and died with
`TypeError: 'NoneType' object is not subscriptable` partway through routing.
Confirmed against the Coello data: Muskingum completes, Kinematic crashes.
The skip is now an explicit argument threaded from the entry point:
`route_muskingum(Model, skip_hydraulic_cells=False)`, set by `Run.run_flood`,
which derives it from `routing_method == "Kinematic"` and takes an override.
`run_distributed` never skips, so the declared method cannot break it.
`"Kinematic"` stays a valid routing method. It names the kinematic-wave scheme
the flood model applies (`SaintVenant.KinematicRaster`, and the routing roadmap
in README), it is documented as such in `docs/api/catchment.md`, and `config.py`
already explains why YAML deliberately does not expose it. Only *where it is read*
changes -- the entry point rather than the inner loop.
`routing_method` also stays. It is not decoration: `hapi.config` cross-checks it
against `parameters.maxbas`, and that check is load-bearing, because a MAXBAS set
holds 11 parameters and a Muskingum set 12 while `maxbas` decides which count is
expected -- so a set contradicting the routing passes the count check and the run
then reads the Muskingum X as the MAXBAS value, giving a quietly wrong hydrograph.
It is dropped from the `DistributedModel` protocol, since the distributed path no
longer reads it, and added to `FloodModel`, which does.
…Kinematic costs
`tests/FloodModel.py` left this repository in `733957be` ("move flood model to
serapis", 2024-01-03) and still sits in Serapis, byte-identical apart from one
`os.chdir` path. It cannot run against this package any more: it points at
`F:/02Case-studies/`, imports the pre-rename `Hapi` package, passes a module
where `read_lumped_model` wants a class, and calls five readers that no longer
exist -- `read_flow_acc`, `read_flow_dir`, `read_rainfall`, `read_temperature`
and `read_et`, all of which moved into `FlowNetwork` and `MeteoInputs`.
Ported to `examples/hydrological-model/coello/run/coello-flood-model-run.py`
against the five river-geometry rasters already in the example data (dem4000,
bankfulldepth, river_width, channel_roughness, floodplain_roughness). It runs.
Running it also showed what the Kinematic path now does. Declaring
`routing_method="Kinematic"` tells the Muskingum pass to leave the river cells to
a 1D hydraulic model -- the handoff the flood model was designed around -- but
that model went to Serapis in the same commit, so nothing here picks them up. On
the Coello example that is 21 of 89 catchment cells and 92% of the discharge,
absent from the results with nothing said. It was unreachable until the previous
commit, because the comparison crashed on a `bankfull_depth` of None; making it
reachable turned a crash into a silent hole.
So the derived case warns, naming the counts and both ways out. An explicit
`skip_hydraulic_cells=True` is a statement that something downstream takes the
cells and stays quiet -- that is the supported workflow, and it should not nag.
The counts are taken inside the flow-accumulation mask. The bankfull-depth raster
carries values outside the catchment too, and counting the raster alone had the
message claiming more river cells than the catchment has.
`Catchment` was both things at once. It is a builder -- constructed empty, filled
by `read_*` calls in any order, some of which a given run never needs -- so every
input on it is `X | None`, and that is honest. The run layer needs the opposite: a
catchment that is finished. Conflating the two had three costs, and only the first
was cosmetic:
1. The engines dereferenced `X | None` on every line, which is why `run`,
`wrapper` and `distrrm` were excused from mypy.
2. "Has this been validated?" was answered by remembering which entry point you
came through. `Calibration` went straight to `Wrapper` and so skipped every
check `Run` performed -- on the one path that rebuilds the parameter array
thousands of times.
3. The engines wrote results back onto the object they read, so a half-finished
run and a finished one looked alike.
`hapi.runs` adds `DistributedRun` and `LumpedRun`: frozen, non-optional, and
reachable only through `from_model`, which is now the single validation seam --
constructing one *is* the validation. The engines take these, so the check is
enforced by the signatures rather than by discipline: `Calibration` cannot bypass
it any more because there is nothing else to pass. `hapi.protocols` keeps the
builder side honest as `CatchmentLike`, whose fields really are optional, and
`Catchment` satisfies it cleanly -- verified with mypy, where it previously failed
on five `| None` conflicts.
Results now only ever come back as return values. `Wrapper.run_lumped` puts the
lumped total in `results.q_total` -- which is what `q_total` means -- and the entry
point indexes it by the period, so `results` is the sole write to the model.
`RiverGeometry` (`hapi.inputs`) holds the five hydraulic rasters that were five
loose attributes assigned by a loop that checked nothing: not that they shared a
shape, not that they covered the catchment. Both are settled now, the first where
the file names are still in hand.
Taking `hapi.wrapper` off the suppression list surfaced four real defects it had
been hiding, all fixed here: `Lake.Qlake` and `Lake.QlakeR` were created by
assignment from inside the wrapper and declared nowhere; `Lake.Parameters` and
`Lake.OutflowCell` were indexed straight while typed optional; and
`ConceptualModelSetup.model` was optional though `read_lumped_model` always builds
one. `DistributedRun` also exposes `parameter_cube` and `routing_table`, so the
engines index a real array and a real dict instead of a union.
`run`, `wrapper` and `distrrm` are off the mypy suppression list; the whole package
type-checks clean. 588 tests pass, 18 of them new and covering the seam itself.
BREAKING CHANGE: `Wrapper.*` and `DistributedRRM.*` take a `DistributedRun` or
`LumpedRun`, not a `Catchment` -- build one with `DistributedRun.from_model(model)`.
`DistributedRRM.route_muskingum` / `route_maxbas` / `route_maxbas_by_path_length`
also take the `SimulationResults` to fill. `Catchment.DEM`, `.bankfull_depth`,
`.river_width`, `.river_roughness` and `.flood_plain_roughness` are gone; read them
off `model.river_geometry`. `Wrapper.run_lumped` no longer sets `Qsim` -- it fills
`results.q_total`, and `Run.run_lumped` puts the frame on the model.
`Calibration` subclassed `Catchment`, which meant it inherited a forty-attribute builder in order to use a dozen fields of it, and inherited `plot_hydrograph` -- which reads `Qsim.loc[...]` and so could never work against the bare array this class's own `extract_discharge` produces. That was a Liskov break with a concrete failure, and it was reachable. Composition does not fix it so much as make it impossible: nothing is inherited, so nothing can be inherited broken. `Calibration(model)` now takes the catchment it calibrates, which also means a model from `Catchment.from_yaml` can be handed straight over. The search space moves with it. `ParameterBounds` is read by nothing but a calibration, and it carries the `(snow, maxbas)` pair every trial vector is checked against, so `read_parameters_bound` and `bounds` belong beside the optimiser rather than on the model being optimised. `Catchment.__init__` is down to 19 attributes, from 42 when this started. Two bugs surfaced while doing it, both of the same kind and both now fixed. The narrowing added in the previous commit was inside the objective function's bare `except:`, so a validation failure was caught and scored `nan` -- the optimiser then searched on over a model that never ran. The narrowing now happens *outside* the try, because a wrong-width vector or a grid mismatch is a defect to surface rather than an infeasible candidate; and the three bare `except:` clauses are `except Exception` with a warning naming what was swallowed, so a long calibration can also be interrupted again. That immediately exposed the second: `calibrate_maxbas` was calling `Wrapper.run_maxbas(run)` with `run` never assigned, and its tests passed because the `NameError` was swallowed as an infeasible trial. Two `TestLumpedCalibration` tests were passing the same way -- they never called `read_lumped_model`, so every trial failed while the stubbed optimiser returned its canned result regardless. Both are fixed rather than papered over. `hapi.calibration` stays on the mypy suppression list, and the comment now says why rather than promising assert helpers: it reads the builder's genuinely optional attributes, and its `SpatialVarFun` argument is typed `Callable` though the code uses `.Function` / `.Par3d` / `.no_parameters` on it. A Protocol for that contract would close most of the remaining 44 errors. BREAKING CHANGE: `Calibration(name, start, end, ...)` becomes `Calibration(Catchment(name, start, end, ...))`, and `Calibration.from_yaml` is gone -- use `Calibration(Catchment.from_yaml(path))`. Everything inherited from `Catchment` is reached through `.model`: `calibration.model.read_parameters(...)`, `calibration.model.meteo`, `calibration.model.QGauges`. `read_parameters_bound` and `bounds` move from `Catchment` to `Calibration`.
…ish calibration's types The calibration entry points typed `spatial_var_fun` as `Callable[..., Any]`, which was doubly wrong: a calibration never calls it, and what it actually does is read four members off it -- `Function`, `Par3d`, `no_parameters`, `no_elem`. So the annotation described a function while the code used an object, and there was nothing for mypy to check the four accesses against. `SpatialDistribution` states them. `Parameters` now provably satisfies it, which took two annotations there: `strategies` is typed `dict[int, Callable[..., Any]]` and `Function` declared rather than inferred, or mypy types the attribute from its first assignment and then rejects both the HRU reassignment below it and the protocol itself. With that and a set of narrowing accessors -- `_search_space`, `_objective`, `_gauged_results` -- `hapi.calibration` type-checks clean and comes off the suppression list, which leaves only `hapi.catchment`: the builder itself, where `X | None` is the honest description. Every module that *consumes* a catchment now narrows first. The accessors also replace a dozen reads that would have failed on `None` with errors naming the reader to call, and mypy caught two real problems on the way: `Catchment` had stopped satisfying `CatchmentLike` because the previous commit moved `bounds` off it without updating the protocol, and `run_calibration` still carried an inline copy of the grid checks that `DistributedRun.from_model` owns. That second one needed care. Deleting the inline copy moved the failure from before the optimiser was built to its first trial, which a test correctly objected to -- a search that cannot complete should not start. So `_check_before_optimising` calls the same seam early rather than repeating its checks, and reports a grid mismatch ahead of a missing objective function, because the first is a data problem the caller can act on. Three more tests were passing on swallowed failures, in the same way the previous commit found two: they never read an objective function or any observed discharge, so every trial raised on `None` and the assertions were reading the stubbed optimiser rather than a run. Fixed by giving them the inputs a calibration needs.
…nning the grid
Muskingum routing must visit cells upstream-first, and flow accumulation gives
that order. `route_muskingum` implemented "process the cells at level j" as "walk
every cell in the grid and test whether it is at level j" -- once per level. Since
the number of distinct accumulation levels grows with the domain, that made the
pass O(n_acc x rows x cols), effectively quadratic, to visit each cell once.
`FlowNetwork.cells_by_acc_val` groups the in-domain cells by accumulation value,
cached beside `acc_val` and dropped with it when the array is replaced. Both loops
now iterate the buckets, so the pass is O(no_elem).
Measured rather than argued:
- bit-identical output. `quz_routed`, `qlz_translated` and `q_total` all compare
`array_equal` against the previous loop, diffed by loading the engine as it
stood at the parent commit and running both over the Coello example.
- Coello: 4,004 cell tests -> 89, and the pass 9.86 ms -> 2.18 ms (4.5x).
- the saving grows with the grid, because what was removed is the quadratic
term: 169x at 13x13, 2,500x at 50x50, 62,500x at 250x250, where 2.5 billion
tests become 40,581.
Order within a level stays row-major, matching the `x`-outer/`y`-inner scan it
replaces -- that is what makes it bit-identical, since the routing accumulates and
a different order inside a level could change the result. Four tests pin it: the
buckets partition the domain exactly, the order is row-major, the visit count is
linear, and the cache is invalidated with the array it derives from.
The `# TODO parallelize` above the loop is now straightforward to act on: each
bucket is an explicit, independent work list rather than a predicate spread over a
grid scan.
One thing found and deliberately left: the loop compares a float raster against
`acc_val`, whose entries are truncated ints, so a fractional accumulation value
(1.2 against the code 1) matches nothing and that cell is never routed. Both
shipped datasets are integral, so it is latent. The index keys on the raw value so
this behaves exactly as before -- fixing it would change results, which is a
separate decision from this one.
`FlowNetwork.acc_val` truncates to integer codes -- `np.unique(_to_int_codes(...))` -- but the routing selected cells by matching those codes against the *raw* accumulation value. So a raster holding 1.2 produced the code 1, `1.2 == 1` was False, and that cell was never routed. Worse than one cell reading zero: the routing sums `quz_routed` from each cell's upstream neighbours, so an unrouted cell contributed nothing to anything below it, and its whole tributary vanished from the hydrograph all the way to the outlet. Nothing raised. Demonstrated on a 2x2 domain holding [0.0, 1.2, 2.5, 3.0]: the loop selected 2 of the 4 domain cells and dropped (0, 1) and (1, 0). Not a design decision. `_to_int_codes` replaced a per-cell `set(int(...))` that truncated on *both* sides of the comparison; somewhere the lookup kept the truncation and the match did not. `acc_val` still documents the original intent -- "1.2 and 1.8 are one code" -- which only makes sense if such cells are then routed together at that code. They were not; they were skipped. So the docstring described the half that no longer happened. `cells_by_acc_val` now keys on the truncated code, restoring that contract: values sharing a code share a bucket and route together. Integral rasters are unaffected, re-verified bit-identical against the engine as it stood before the bucketing change -- and both shipped datasets are integral, so nothing that ships changes behaviour. What changes is a weighted accumulation raster, or one some tool exported as float, which previously lost cells in silence.
…lear the plan Three remaining items from the coupling plan, all small. `FlowNetwork` promised its two rasters share a grid but only checked at construction: `__setattr__` dropped the derived caches and let a replacement of any shape through, so the accumulation and direction arrays could end up describing different catchments and a cell index would mean a different place in each. It now calls a `_check_replacement` mirroring the one `MeteoInputs` has always had. That makes the shape check in `DistributedRun.from_model` unreachable, so it is gone. It only ever existed to compensate for this gap -- the architecture review predicted it would become dead once the hole closed, and it has. The test that staged a mismatched raster to exercise it now asserts the refusal at the point of assignment, which is where the mistake is made. `read_discharge_gauges` reads `self.period.date_index` instead of re-deriving the daily/hourly branch, which was the last of four hand-written copies. `Wrapper` and `DistributedRRM` lose their do-nothing `__init__`. Both are namespaces of static methods and neither was ever instantiated. Verified: a mismatched replacement raises, a same-shape one is still allowed, and adding a direction raster to a network built without one still works -- the last mattering because MAXBAS builds networks with no direction raster at all.
…d it Half the memory a distributed run allocates was `state_variables`, a `(rows, cols, steps, 5)` array -- as much as `quz`, `qlz`, `quz_routed`, `qlz_translated` and `q_total` put together. Nothing reads it but `save_results` and `plot_distributed_results` options 4 to 8. The routing does not touch it, and neither does `extract_discharge` or any calibration. `DistributedRun.keep_state_variables` makes it optional. It defaults to True, so no existing caller changes behaviour or memory. `Calibration` passes False, which is where this matters: it runs the model once per trial vector, thousands of times per search, and never looks at the states. Measured on the Coello example: result arrays 78 KiB -> 39 KiB, exactly half, with all five discharge fields `array_equal` either way. Projected on a 500x500 grid over a decade of daily steps, 34 GiB -> 17 GiB. The state options now go through `_require_state_variables`, which names the switch instead of failing on `None` inside a slice several frames away. It is called at each option rather than bound once at the top of the method, so a discharge-only plot on such a run still works -- covered by a test, since binding it eagerly is the obvious way to write this and is wrong. Scope, plainly: this is the achievable half of ARC-HAPI-12, not the streaming the item originally proposed. Streaming the time axis is not reachable from here -- HBV's `simulate()` runs the whole series for one cell at a time, and the routing needs every cell's full series in upstream-to-downstream order, so no cell's history can be dropped mid-pass. Chunking time would mean restructuring the conceptual model to accept and return partial state, which is separate work. What is left of the ceiling is 5 cube-equivalents instead of 10.
`Catchment` is a builder: it assembles inputs and hands them to the run layer.
It also carried four methods that do the opposite -- take finished results and
turn them into figures and files. Three of them, 332 lines, were the only reason
the builder imported a plotting stack at all, and the only reason it held `anim`
and `_animation_glyph`, two of its nineteen attributes.
They read result arrays. They now live on the object that holds those arrays:
* `plot_distributed_results` -> `SimulationResults.animate`
* `save_animation` -> `SimulationResults.save_animation`
* `save_results` -> `SimulationResults.save`
`catchment.py` drops from 1,760 to 1,428 lines and no longer imports cleopatra,
`DatasetCollection` or `matplotlib.animation`; `Catchment.__init__` goes from 19
attributes to 17.
The methods need the calendar to index the arrays by, the grid to mask them with
and the drivers to animate beside them, so `SimulationResults` now carries the
`DistributedRun` or `LumpedRun` that produced it. That is provenance the object
wanted anyway -- the arrays are not interpretable without it -- and it is the one
part of this that is a design decision rather than a move. Results built by hand
rather than by a run say what is missing instead of failing on `None`.
cleopatra is imported *inside* `animate`, not at module scope. `hapi.results` is
what every engine imports, so a top-level import would have put matplotlib in the
path of every model run -- worse than the arrangement it replaced.
`test_running_a_model_does_not_import_a_plotting_stack` holds it there.
Three things fell out of the move rather than being aimed at:
1. `save` reads rasters-or-CSV off `routing` instead of the caller's
`spatial_resolution`, so the choice is a property of the results rather than
something restated at the call site.
2. The CSV branch built its index with a hard-coded `freq="D"`, so an hourly
lumped run wrote a daily index against hourly values. It uses the run's own
calendar now.
3. `gauges` was a `bool` that reached back onto the catchment for `GaugesTable`.
It takes the table itself, which is what let the method stop knowing about
catchments at all.
`plot_hydrograph` deliberately stayed on `Catchment`. It reads no result array --
it plots `Qsim` against the observed gauge record, and `Qsim`, `metrics` and
`QGauges` are analysis products, not run output.
`SimulationResults` is now documented in the API reference, which it was not
before. 602 tests pass plus 17 plot tests and 36 doctests; mypy clean.
BREAKING CHANGE: `Catchment.plot_distributed_results`, `.save_animation` and
`.save_results` are gone. Call them on the results a run returns:
`model.results.animate(start, end, option=1)`,
`model.results.save_animation(path, fps=2)` and
`model.results.save(path, result=1, flow_acc_path=...)`. `save`'s first positional
argument is now `path`, not `flow_acc_path`, and its date arguments are `start` and
`end`. `animate`'s `gauges` takes the gauge table (`model.GaugesTable`) rather than
a `bool`. `Catchment.anim` and `Catchment.STATE_VARIABLES` moved to `hapi.results`.
`hapi.results` sat at 90% line and branch coverage after the presentation methods
landed on it. The happy paths were covered -- they came with the methods -- but
thirteen branches were not, and each one is a place where a caller gets an answer
they did not ask for rather than an error:
* a lumped run asked to animate, which has no grid and no drivers;
* a routed field read before its routing step filled it;
* `start` / `end` given as `datetime` rather than `str`;
* a date that is not a step of the run at all -- `np.nonzero(...)[0][0]` raises
`IndexError: index 0 is out of bounds`, naming neither the date nor the span;
* rasters asked for with no georeferencing template, or an option outside 1-8;
* a caller's raster prefix, which every existing test left at the default;
* the lumped CSV options 2, 3 and 4, and an option outside 1-5, which would
otherwise write a file holding nothing but a `date` column.
`tests/rrm/catchment/test_results.py` takes the ones that are about the object
itself; the three raster guards go beside the fixture that already builds rasters
in `test_save_results_distributed.py`. `hapi.results` is now at 100% line and
branch, and the suite is 627 in the main task plus 17 in `plot`.
…t drifted `mkdocs build --strict` failed on thirty griffe warnings, every one of them a docstring describing a signature that no longer exists. `Wrapper`'s five entry points still documented `Model`, `ll_temp`, `q_0` and `skip_hydraulic_cells` -- parameters removed when the engines started taking a `DistributedRun` or a `LumpedRun` -- and three of them told the reader that results are "stored directly on the Model object", which is the exact behaviour that change removed. They document `run` now, and say what they return. The other eleven were a bullet list under `Returns:` whose wrapped lines sat at six spaces where griffe wants a multiple of four. `mkdocs build --strict` is clean. `SimulationResults.animate`, `save_animation` and `save` arrived here with no `Examples:` at all. Each has one now, and they run: the doctest task covers this module, so `save`'s example builds a small `LumpedRun` and writes a real CSV, and the reader sees the actual columns and the date quoting rather than a description of them. The rendering path is not doctested -- it needs a raster grid and a plotting backend -- and stays covered by `test_plot_animation.py`. `animate` also imported cleopatra as its first statement, so a rejected option or an out-of-range date paid for loading matplotlib before being told it was wrong. The import sits below the checks now, which is what makes the first example true. 39 doctests pass, 3 skipped.
… just the routed ones `Run.run_lumped(model)` -- the entry point called with the `Route=0` it declares as its own default -- raised `ValueError: Length of values (1096) does not match length of index (1095)`. The conceptual model prepends an initial-state slot, so its series is one step longer than the period covers. Both routing branches trimmed it with `q_total[:-1]`; the unrouted path did not, so the total stayed `n + 1` long and could not be indexed by the period. The trim now happens once, above the branches, which is where it belongs: the length is a property of the run, not of which branch it took. The routed paths are unchanged -- they were already trimming the same slot, one line lower. `test_maxbas_routing_convolves_qsim_with_the_last_parameter` had encoded the defect as intended behaviour: it shortened the unrouted reference series itself, and its comment explained that "the unrouted one is a step longer than the index, so only the routed form survives that call". It compares like with like now. Two regression tests cover what nothing covered: the default flag runs and produces a series the period can index, and turning routing on does not change the length.
`RoutingKind` is presented throughout the package as a property of the arrays --
"the routing is a property of the arrays, so it is read off them instead". It was
not. Only `route_muskingum` set it. MAXBAS was labelled by
`Wrapper._set_maxbas_output_fields`, one layer *above* the router, and
`route_maxbas_by_path_length` -- a public, documented entry point that nothing in
the package calls -- set nothing at all.
So a caller driving `DistributedRRM` directly, which is the pattern
`docs/api/distrrm.md` documents, got MAXBAS-routed arrays still labelled
`UNROUTED`, and `outlet_shortcut_valid` answered `True` for them -- offering the
outlet-cell shortcut for a scheme that makes a cell a contribution rather than a
discharge. Today that fails loudly because `q_total` is still `None`; it is one
line of future plumbing away from being silently wrong, which is the failure mode
this whole design exists to prevent.
The helper moves down into `DistributedRRM._record_maxbas`, beside the routing it
describes, and both triangular routers call it. `Wrapper` no longer labels anything
it did not route.
Two related holes closed with it:
* `outlet_shortcut_valid` now excludes `UNROUTED` as well as `MAXBAS`. There is no
`q_total` on unrouted results, so there is no cell to read and no shortcut to
take.
* `extract_discharge` refuses unrouted results by name, rather than picking a
branch on that property and failing on `None` several frames in with a message
about an array.
Six tests cover it: each router records itself, the shortcut is valid for exactly
the two schemes where a cell is a discharge, and extracting before routing says
which step nobody ran.
…ller `opt_fun` catches `TypeError` from the objective call and re-raises it as the "objective function you have entered needs more inputs" error. That `raise` sat *inside* the `try` that classifies a trial as numerically infeasible, so the `ValueError` it raised was caught by `except Exception` one line later and scored `np.nan` with `fail=1`. The result: a caller who wired up an objective with the wrong signature got a full Harmony Search over an all-`nan` landscape and one warning per trial, never the message the constant was written for. Narrowing the bare `except:` to `except Exception:` earlier in this branch fixed the `KeyboardInterrupt` problem but not this one -- the error is a `ValueError`, and `Exception` catches it either way. `ObjectiveFunctionArityError` gives it a type of its own so it can travel through that handler, and each of the three entry points re-raises it above the numerical case. It stays a `ValueError` subclass, so anything already catching `ValueError` around a calibration is unaffected. Two tests, one for each side of the handler: an objective of the wrong arity now reaches the caller, and an objective that fails on the *values* is still scored infeasible and still lets the calibration continue -- which is what the handler is for, and what a careless re-raise would have broken.
`DistributedRun.from_model` is documented as the single seam where everything checkable is checked, and it checks the drivers, the parameter cube and the river geometry against the grid. The flow-path-length raster was carried in with a bare `getattr` and compared to nothing -- `read_flow_path_length` does not compare it either, having deliberately stopped deriving `rows`/`cols` from it. `route_maxbas_by_path_length` then indexes it by `flow_network.rows`/`cols`, so a raster on a different grid either raised `IndexError` several frames inside that loop or, if it was larger, quietly read the wrong cells for every cell of the catchment. Three shapes are refused and the matching one is accepted, so the guard is shown doing both halves of its job.
The guard admitted any lake record with at least three columns and its message named "rain, ET, and Temp". Both lake wrappers then read `meteo_data[:, 3]` for the long-term average temperature, so a three-column record passed validation and raised `IndexError` inside the run -- naming a column index rather than the driver nobody supplied. The check and the message now ask for the four columns the code reads. The test is parametrised over two and three columns: three is the width that regressed, two is kept so the guard is still shown refusing what it always refused, and both now assert the engine is never reached.
…outing path `SimulationResults.qout` is documented as one thing -- "the outlet hydrograph" -- but its length depended on how the run had been routed. The MAXBAS and lake paths trim the conceptual model's leading initial-state slot and return `len(period)` values; the Muskingum path read the outlet cell of `q_total` whole and returned one more. Anything indexing `qout` by the period therefore worked on one routing path and raised on the other. The Muskingum branch trims like the rest, and the field's docstring now states the length rather than leaving it to be discovered. A test pins that it covers the period exactly.
…clare `ParameterBounds.__post_init__` compared `len(lower)` with `len(upper)` and stopped there, while its own docstring says "the bounds are where the configuration enters, and every trial vector the optimiser produces is checked against it". The bounds themselves were not checked against `PARAMETER_COUNTS[(snow, maxbas)]`. So a ten-value bound list with `snow=False, maxbas=False` built fine, and the mismatch surfaced once per trial from `ParameterSet.__post_init__` -- after the whole optimisation problem had been declared and the optimiser started, from inside the objective, rather than at the call that got it wrong. It calls the same `validate_parameter_count` every trial vector goes through.
…used buffer `SimulationResults.run` is documented as "the validated inputs these arrays came from, carried as provenance". In a calibration it was not: `SpatialVarFun.Function` fills the *same* `Par3d` buffer on every trial, and `_parameter_set` wrapped that buffer by reference. Every `ParameterSet` -- and every results object reached through it -- therefore held a view of an array the next trial overwrote in place, so `results.run.parameters.values` described whichever trial happened to run last rather than the one that produced those arrays. One copy per trial, against a model run per trial, so the cost is not the point of comparison here; the alternative was a provenance field that cannot be trusted on the one path that produces thousands of result objects.
`SimulationPeriod` is frozen precisely so its derived values cannot drift from the inputs they come from, which also makes them safe to memoise. `date_index` rebuilt its `pd.date_range` on every read anyway, and `days` and `__len__` go through it. `DistributedRun.__post_init__` and `LumpedRun.__post_init__` each read it once per calibration trial, and `SimulationResults._step_bounds` reads it twice per call. `cached_property` is what `FlowNetwork.acc_val` and `cells_by_acc_val` already use for the same reason.
The `Catchment` class docstring said the result arrays "are also readable under their historical names (`q_total`, `quz`, ...) as read-only properties forwarding to it". No such properties exist -- `grep "@Property" src/hapi/catchment.py` finds none. That plan was written down when the results object was introduced and then deliberately dropped: the whole point of the redesign is that `results` is the only home for the arrays. The sentence was not harmless. `coello-distributed-model-run-netcdf.py` is built on it: its loop was renamed from `Qtot` to `q_total` but left pointing at the catchment, so the shipped script raised `AttributeError: 'Catchment' object has no attribute 'q_total'` for all three fields. It reads them off `Coello.results` now, and the script runs end to end. Three copies of the same claim went with it -- the `Outputs: ... [numpy attribute]` blocks in two run scripts and `tests/run/distributed_mode_run.py`, which named the arrays as attributes of the model, and the sentence in `docs/examples/distributed-model-calib.md` saying the results "will be stored as attributes in the Catchment object".
…ts they replaced `docs/api/results.md` was the only page this branch added, but `SimulationPeriod`, `DistributedRun`, `LumpedRun`, `ParameterSet`, `ConceptualModelSetup`, `ParameterBounds`, `CatchmentLike` and `SpatialDistribution` are all public now -- `Catchment.period`, `.parameters` and `.model_setup` are typed with them, and docstrings across the package carry dozens of `hapi.runs.DistributedRun` style cross-references that resolved to no page at all. Four pages, each opening with why the object exists rather than only what it holds: the builder/finished split for `runs`, the six-attributes-describing-one-thing story for `period`, the per-object invariants for `conceptual`, and dependency inversion for `protocols`. `CONVERSION_FACTOR` and `PARAMETER_COUNTS` go with them. Both were dead in `catchment.py` -- only the definitions were left -- but they were still public module-level names, and a second source of truth for the two rules the refactor was consolidating: the mm-to-m3/s factor, which now lives in `period`, and the `(snow, maxbas)` count table, which lives in `conceptual`. The next person to change one would not have thought to change both. `mkdocs build --strict` stays clean.
…ut result views Every raster read here opened a GDAL dataset and left it open. On Windows an open handle keeps a lock on the file, so a script that reads a catchment and then moves or rewrites its inputs fails, and a loop over basins -- or a repeated `results.save` -- accumulates handles for the life of the process. The four sites this branch wrote or rewrote now read inside a `with`: `SimulationResults._save_rasters`, `FlowNetwork.from_rasters` (two handles), `RiverGeometry.from_rasters` (five) and `Catchment.read_flow_path_length`. Everything they need is copied into arrays, so nothing wanted the handle afterwards. `_save_rasters` also handed `Datacube.values` a `np.moveaxis` view straight onto a result array. Nothing writes through it today, but a pyramids version that normalises no-data in place would edit the arrays a *save* is only supposed to read. It writes a contiguous copy. The related aliasing that stays is now stated where a reader will meet it: after a MAXBAS run `quz_routed` *is* `quz` rather than a copy of it, because the triangular routing works in place and a copy would double the memory of a `(rows, cols, time)` array for nothing. The field docs say so, since `results.quz_routed is results.quz` is otherwise invisible from outside.
…igrated Eleven notebooks were swept by the CamelCase rename. All eleven still import the pre-rename `Hapi` package, so none of them can run and none of the edits changed anything that executes -- the `notebooks` pixi task's own description already records that they need migrating first. Four of them came out worse. `from Hapi.run import runHAPIwithLake` became `from Hapi.run import run_distributed_with_lake`, which turned an obviously stale line into one that looks current and still cannot work twice over: the package is `hapi`, and `run_distributed_with_lake` is a `staticmethod` on `Run`, never a module-level function. A reader can see that the first is old; the second reads as maintained. Reverted to their state on `main`. Migrating them properly -- package name, entry points, and the results object -- is its own piece of work.
The branch's stated position is that everything is `snake_case` everywhere and that `_maxbas_routed` is gone. The test suite still carried `TestDistMaxbas2`, `TestFW1Calibration`, `TestRunFloodModel`, `TestRunHapiWithLake`, `TestRunHapiWithLakeEndToEnd`, `test_run_fw1_returns_maxbas_routed_results` and a `test_marks_the_model_as_maxbas_routed` whose docstring still explained what the flag did -- which is the first place someone greps for a name they cannot find. Renamed to the entry points that survive, and the two docstrings that described the flag now describe the routing recorded on the results. The remaining mentions of `_maxbas_routed` are in module docstrings explaining what it was replaced by, which is the one place the old name still earns its keep.
… assumes Six small things the review turned up, all in code or prose this branch wrote. `animate`'s docstring listed option 2 as "Upper zone discharge" and option 3 as "Ground water" while the options themselves are titled "Surface Flow" and "Ground Water Flow" and read `quz_routed` / `qlz_translated`. The list now says what the options do, and which ranges are states and drivers. The no-data mask writes NaN into a copy of the selected array. The meteo options read `MeteoInputs` cubes, which are documented as carried through "as stored", so an integer driver raster would raise `cannot convert float NaN to integer`. It copies to float32 when the dtype cannot hold NaN, and leaves float arrays alone rather than upcasting them. `_save_csv` tested the same kind of membership two different ways in adjacent lines. `read_lumped_model` instantiates the conceptual model, so `ConceptualModelSetup` holds an *instance*; the `save` doctest passed the class, which happened to work because nothing called it, and contradicted the example in `conceptual.py`. `docs/api/catchment.md` said "up to and including version 1.7.0" about a check this branch adds, while `pyproject.toml` still reads 1.7.0 -- it described the current release as the past one. Three added lines were over the repository's 120-character limit.
MAfarrag
force-pushed
the
fix/example-yaml-literal-paths
branch
from
September 10, 2026 21:00
2ed3fee to
67e5de0
Compare
Round 1's fixes added or changed twelve guards, and coverage on the modules they
live in ranged from 91% to 96% -- the new checks were exercised, but the branches
around them were not. All seven are at 100% line and branch now:
conceptual 93% -> 100% runs 92% -> 100%
period 96% -> 100% wrapper 91% -> 100%
distrrm 96% -> 100% run 96% -> 100%
results 100% -> 100%
What was untested and now is: the parameter cube's *columns* check (only rows had a
test, so half the guard was unexercised); the skip-without-geometry guard on
`DistributedRun.__post_init__`, which `from_model` had always refused earlier; a
direction raster carrying no lookup table; `routing_table` on a network built
without one; a backwards simulation span; an initial condition that is not five
values; bounds of different lengths; `ParameterSet.count`; the path-length router
with no raster; lumped routing asked for with something that is not callable; a
lake with no record at all; and each of the three lake inputs `_lake_inputs` names.
One of these is worth more than the coverage number: nothing exercised the
*taken* branch of `skip_hydraulic_cells`. The flood tests check that the warning
fires, not that the cells come out unrouted -- and the cell has to be downstream,
because a headwater is copied across before the skip is consulted. That test now
pins the handoff itself.
`if __name__ == "__main__": print("Wrapper")` and its twin in `run.py` are deleted
rather than excluded: vestigial module-run scaffolding that printed a class name,
the same thing the do-nothing `__init__`s were.
685 tests in the coverage run, 668 in the main task plus 17 in `plot`; doctests,
mypy, ruff all clean.
…nder doctest
`Catchment.extract_discharge` gained a guard this round -- it refuses results no
routing step has filled -- and its `Raises:` section did not mention it. That is
the one correctness item here: a documented contract that had stopped matching the
code.
The rest is the symbols whose *meaning* changed, each with an executable example
rather than a description of one:
* `SimulationResults.outlet_shortcut_valid` -- it now excludes `UNROUTED` as well
as `MAXBAS`, so the example shows which kinds do and do not support reading the
outlet cell.
* `RoutingKind` -- the values a run records on its results.
* `SimulationPeriod.date_index` -- cached now, so the example pins that the same
object comes back; `freq` and `conversion_factor` got theirs alongside, the
latter showing the factor of 24 between the resolutions.
* `ParameterSet.count` -- reads back the width the set was checked against, which
for a distributed cube is the parameters per cell, not the cells.
* `ObjectiveFunctionArityError` -- new public name this round. Its example shows
the property the design turns on: it is still a `ValueError`, so code already
wrapping a calibration in `except ValueError` keeps catching it.
`src/hapi/calibration.py` joins the `doctests` task, so those examples are checked
rather than trusted. 46 doctests pass, up from 39; `mkdocs build --strict` stays
clean.
Not done, deliberately: `Run.*`, `Wrapper.*`, `DistributedRRM.*` and the
`Calibration` entry points still carry no examples. Every one needs a raster
dataset and a full run to demonstrate, which is why their modules sit outside the
doctest task -- an example there would be prose that drifts. They are covered by
the test suite instead.
…l's width Round 1 added `validate_parameter_count` to `ParameterBounds.__post_init__`. That was wrong, and it broke every distributed calibration. A `ParameterSet` and a `ParameterBounds` bound two different things. A parameter *set* is what the conceptual model reads: 12 values per cell for `(snow=False, maxbas=False)`. The *bounds* delimit the flat vector the optimiser searches, whose length is the spatial distribution's `ParametersNO` -- `no_elem * no_parameters (+ no_lumped_par)`. On the shipped Coello grid that is **980** totally distributed and **243** for HRUs. All three were refused. There was no width that worked: pass 12 to satisfy the new check and `par3d` then raises `length of input parameters should be 89*(12 - 1) + 1 = 891`. The two coincide only for a lumped calibration, where the trial vector *is* the parameter set -- so the rule moves to `calibrate_lumped`, before the optimisation problem is declared, which is where round 1's concern (a mismatch surfacing once per trial from inside the objective) is actually true. Nothing caught this because every distributed-calibration test hand-assigns `ParameterBounds(np.zeros(12), np.ones(12))` rather than a realistic search space, and round 1's own test parametrised three *lumped* widths. It now asserts the opposite for the distributed case -- 12, 243 and 980 are all accepted -- keeps the length-mismatch rule, which holds whatever the search width is, and covers the lumped check on the path where it belongs.
…from a TypeError Round 1 made the "objective needs more inputs" error escape the handler that scores a trial infeasible. The diagnosis behind it is a blanket `except TypeError` wrapped around the objective call *and* the Muskingum constraint loop after it. While the error it raised was swallowed one line later that over-catch was harmless -- a misclassified trial was still just a `nan`. Once it escaped, any `TypeError` raised for a *value* reason inside a correctly wired objective ended the whole search and blamed the signature. The objective is user-supplied and is handed two pandas frames, so a `TypeError` there is not exotic. Arity is a property of the wiring, not of a trial: it is knowable before the search starts. `_check_objective_arity` binds the objective's signature against the number of arguments its entry point passes, and each of the three calls it once, up front. A wrongly wired objective is now reported before a single trial runs -- earlier than round 1 managed -- and the per-trial handler goes back to treating every runtime failure as one bad candidate. The check immediately found a wrongly wired test: `TestCalibrateMaxbas` passed `metrics.rmse`, which takes two arguments, while `calibrate_maxbas` calls `objective(QGauges, qout, GaugesTable)`. Every trial in that test raised `TypeError` and scored `nan`; it passed anyway because it only asserted on the stubbed optimiser's canned result. It uses a three-argument objective now. Two tests hold the two halves apart: an objective of the wrong arity reaches the caller, and a `TypeError` raised on the values is still just an infeasible trial with the search continuing.
Round 1 diagnosed this correctly -- `SpatialVarFun.Function` wants the optimiser's flat vector, not a `ParameterSet` -- and then wrote the fix into one of three sibling call sites, leaving the keywords wrong even there. `Function` is one of `par3d` / `par3d_lumped` / `hydrologic_response_units` / `par2d_lumped_k1_lake`, and every one of them takes exactly `(self, par_g)`. The `kub`/`klb` keywords were commented out of those signatures years ago, so all three calls raised `TypeError: par3d() got an unexpected keyword argument 'kub'` -- including the one round 1 "fixed". The other two also passed the wrong object: a `ParameterSet` in the totally-distributed script, and its `(13, 14, 12)` cube in the `tests/` one, both products of the mechanical `Coello.parameters` -> `Coello.model.parameters` rewrite. All three pass `Coello.best_parameters` now, with no keywords. The same two scripts also built the distribution with `DP(..., klb=, kub=)`, whose parameters are `k_lower_bound` / `k_upper_bound` -- so they raised before reaching the call above. Verified against the installed class: the constructor takes the long names, `Function(flat_vector)` returns a `(13, 14, 12)` `Par3d`, and `Function(..., kub=, klb=)` still refuses.
Round 1 made the routers record their own routing kind and made `extract_discharge` refuse results nothing had routed. Between those two it opened a gap: `route_maxbas_by_path_length` now labels its results `RoutingKind.MAXBAS`, but unlike `route_maxbas` it has no `Wrapper` entry point to sum the domain after it, so those results arrive labelled MAXBAS with `qout` still empty. That is the one state the new guard cannot see -- it only tests for `UNROUTED`. `np.reshape(None, n)` then reported `cannot reshape array of size 1 into shape (10,)`, naming neither the field nor the step that should have filled it. Before round 1 the same call said `'NoneType' object is not subscriptable`; the fix relabelled the failure rather than closing it. The MAXBAS branch requires `qout` by name now, and the message says where it comes from: the `Wrapper` entry points fill it, because summing the domain is not a routing step. `_record_maxbas`'s docstring says the same, so a caller driving `DistributedRRM` directly knows what they still owe. The round-1 test asserted the routing kind and `q_total` and stopped one field short; it now also pins that `qout` is deliberately empty, and a second test walks the whole path-length route through `extract_discharge`.
Round 1 fixed `run-configuration.md`'s argument order but paired `Routing.triangular_routing_1` with `coello-lumped-model-run.yaml`, whose parameter set declares `maxbas: false`. `Wrapper.run_lumped` picks the routing signature off that flag, so the snippet took the Muskingum branch and called a two-argument function with five: `TypeError: triangular_routing_1() takes 2 positional arguments but 5 were given`. It uses `Routing.muskingum_v` now, matching the shipped script beside it, and says which config to switch to for the triangular function. Executed from the repo root: 1,095 steps against a 1,095-step period. The snippet also loaded the config by a bare filename while every script on the branch uses the repo-root path; it now does too. `distributed-model-run.md` is the one example page the diff never touched, which is why both rounds fixed the four that are in it and left this one. It still built `Calibration(name, Sdate, Edate)` -- a signature that now raises `TypeError` -- and called five readers, `GaugesTable`, `extract_discharge` and `Qsim` on the `Calibration` rather than on `.model`. Four more things were wrong with it independently of that: `gdal.Open` where `Parameters` requires a pyramids `Dataset`; `Function=`/`Klb=`/`Kub=` where the constructor takes `function=`/`k_lower_bound=`/ `k_upper_bound=`; `SpatialVarFun.Function(Coello.parameters, kub=..., klb=...)`, which is the same call H2 corrected in three scripts; and an objective declared with five parameters where `run_calibration` passes two -- which, since this round made arity a checked property, is now reported up front instead of scoring `nan`. Its `read_objective_function` call was indented inside the function body, so it never ran at all.
Three small defects, each introduced or left open by a round-1 fix. `_save_rasters` guarded against handing the raster writer a view of the result arrays with `np.ascontiguousarray`. That returns its input untouched when the input is already contiguous, and numpy ignores size-1 dimensions when testing contiguity -- so a single-step range, which `save(start=d, end=d)` asks for, still passed a view. Measured: a one-step slice was not copied, two and five were. It uses `np.array(..., order="C", copy=True)`, and a test covers both the case that regressed and the case that always worked. `Calibration.extract_discharge` is a second implementation of the same method and did not get the `UNROUTED` refusal `Catchment.extract_discharge` gained. It read only `outlet_shortcut_valid`, which the same round-1 change widened to exclude `UNROUTED` -- so unrouted results reached a message stating categorically that the run used triangular routing. A confident wrong diagnosis is worse than the `AttributeError` it replaced. It makes both refusals now, in the same order, and a test asserts the message does not mention MAXBAS. The Jiboa example passed a directory-plus-prefix as `path`. `save` joins rather than concatenates now, so instead of prefixing the file names it created a directory literally named `Lumped_Parameters_<date>_` and wrote `Result_*.tif` inside it. The branch rewrote that call without updating the value it passes, while the four Coello scripts and both docs pages moved to the directory + `prefix=` form.
…the path-length range `read_objective_function(fn, args)` documents `args` as "extra arguments forwarded to it", and the `_objective()` helper this branch introduced says the same in its own `Returns:`. Two of the three entry points bound them and then never passed them: `run_calibration` and `calibrate_maxbas` called the objective with a hard-coded argument list. A caller who registered extra arguments got no error and no effect, which is the silent kind of wrong -- and ruff cannot see it, because `F841` does not report an unused name from tuple unpacking. All three forward `*of_args` now, and the arity check counts them. `route_maxbas_by_path_length` normalises by `max - min` over the flow-path-length raster with nothing checking that range is non-zero. A constant raster made every cell's MAXBAS NaN, surfacing as "Maxbas value has to be at least 1, got nan" from inside `triangular_routing_2` -- several frames away, naming a parameter the caller never set. It now names the raster and why it cannot be scaled.
The biggest one is mine from round 1. Three docstrings explained the length trim as dropping "the leading initial-state slot" -- `[:-1]` drops the *trailing* element. The conceptual model sizes its arrays `len(prec) + 1` and writes indices 0..n-1, so index 0 holds the initial state, 1..n-1 the simulated steps, and index n is never written. Verified on the shipped lumped configuration: `q_total[0]` equals the untrimmed `q[0]`, and the dropped value is the unwritten `0.0` at the end. The trim is right and the calendar alignment is right; the explanation was backwards, and it hid something worth knowing -- `Qsim.iloc[0]` and `qout[0]` are the warm-up state, not a simulated value, which matters when scoring the first step. `_check_lake_meteo` was raised to four columns this round and every surrounding docstring was left at three -- including the `Raises:` section of the function that changed, both lake entry points, and `Lake.read_meteo_data`. All four name the long-term average temperature now. `save`'s option list still called options 2 and 3 "Upper zone discharge" and "Lower zone discharge" while `_RASTER_OPTIONS` writes `quz_routed` and `qlz_translated`. The same drift was corrected in `animate`'s list this round and this one was missed, so the two lists described the same arrays differently. `docs/api/catchment.md` said `Catchment` *and `Calibration`* accept a routing method; `Calibration` takes a model and no routing method at all. It also justified the spelling check by a comparison the branch deleted -- the routing loop no longer compares against `"Muskingum"`. The page now says what actually reads the stored method: `Run.run_flood` deriving the river-cell skip from `"Kinematic"`, and the cross-check against `parameters.maxbas`. `README.md` said "there is no compatibility alias: the names above are the only ones" directly after listing the *removed* CamelCase names -- reading as if `Run.RunHapi` is what survives, in the one paragraph a downstream reader consults about the break. Two costs are now written down rather than left to be discovered: the per-trial parameter copy is a full cube kept alive by `results.run` (~96 MB per retained result on a 1000x1000 grid), and `SimulationPeriod.dt` notes that the lake paths pass `conversion_factor` into the same `muskingum_v` parameter the catchment paths pass `dt` to -- an 86.4x difference between two routings of the same kind, which is the physics question already filed as issue #218.
…ld constructor `tests/sensitivity_analysis.py` read `UB` from `LB-1-Muskinguk.txt` and `LB` from `UB-1-Muskinguk.txt`, then handed both to `SA(parameters, LB, UB, ...)` -- so every sample was drawn from the range upside down. Pre-existing, but this branch rewrote the two lines below it and both `SA` arguments, so the swap was inside the diff being edited. `distributed-model-calib.md` quoted and then called the pre-rename constructor (`StartDate`, `EndDate`, `SpatialResolution`, `TemporalResolution`) and omitted `routing_method` entirely; it also named `1-statevariables` where the field is `state_variables`, and read its gauges from `Hapi/Data/00inputs/...`, a path that does not exist in the repository and that ignored the page's own `Path` variable. `lumped-model-run.md` imported the *module* `hapi.rrm.hbv_bergestrom92` and passed it to `read_lumped_model`, which guards on `inspect.isclass` and refuses a module; passed `Title=` where the keyword is `title`; and read `Coello.QGauges['q']` on a page that never calls `read_discharge_gauges`, so the attribute was `None`. Five tests added in round 1 landed inside `TestTemporalResolution` and none of them was about temporal resolution. They move into `TestSimulationPeriod` and `TestConceptualModelInputs`, which is where someone would look for them.
…pository `examples/hydrological-model/jiboa/data/` is untracked -- 85 files, about 2 MB -- so the only exercise of `Run.run_distributed_with_lake` outside the unit tests fails on its first read for anyone who clones the branch. `tests/rrm/data/jiboa/` carries the lake record and its parameters and nothing else, which is not enough to drive it. The data stays out of the repository, by decision. The script's docstring now names exactly what it expects and says plainly that it is not shipped, so the failure is explained before it happens rather than surfacing as a missing-file traceback.
…measured
Round 1 took seven modules to 100% line and branch. `calibration.py` was not among
them and sat at 87% -- which is why round 2 found three of round 1's own fixes
wrong there, and why one of them, the `ParameterBounds` width rule, broke every
distributed calibration without a single test noticing.
`tests/rrm/calibration/test_calibration_guards.py` covers what a calibration
refuses before it starts, and the lumped objective loop that nothing exercised:
* `read_parameters_bound` with a `snow` flag that is not a bool -- `0` and `1`
hash equal to `False`/`True`, so an int gets the right answer by accident and a
string fails much later;
* `read_objective_function` with something that cannot be called, and the
`args=None` default the entry points would otherwise try to unpack;
* a missing search space, a missing objective, and each of the three inputs
`_gauged_results` narrows;
* an objective whose signature cannot be introspected, which the arity check has
to decline to judge rather than reject;
* `calibrate_lumped` with an incomplete `basic_inputs`, with no observed record,
with a width the conceptual model cannot read, and with a wrongly wired
objective -- each refused before the optimisation problem is declared;
* and the trial loop itself: a valid trial runs the model, returns a finite score
and both Muskingum stability constraints, while a trial that fails numerically
is scored infeasible and the search continues.
87% -> 97%, with the other seven modules holding at 100%. What is left is
`calibrate_maxbas`'s objective body, which needs a stubbed optimiser driving a
distributed run, and three partial branches.
699 tests in the main task plus 17 in `plot`; mypy and ruff clean.
`read_objective_function`'s `args` were documented as "forwarded to the objective" and two of the three entry points silently dropped them. They are forwarded now, so the docstring says what the caller can rely on -- appended after the arguments the entry point supplies, counted by the arity check, so registering arguments the objective cannot accept is reported before the search starts rather than scored as `nan`. Two runnable examples: the arguments are kept as given, and a non-callable is refused where it is registered. `calibrate_lumped` gained the parameter-count rule this round, and its `Raises:` now says so and why it lives there rather than on `ParameterBounds` -- a distributed calibration searches `SpatialVarFun.ParametersNO` values, so the widths coincide only on this path. It also documents `ObjectiveFunctionArityError`, which it can raise before the optimiser is built. `route_maxbas_by_path_length` refuses a constant flow-path-length raster, and `Catchment.extract_discharge` refuses MAXBAS results carrying no `qout`. Both `Raises:` sections name the new condition and, for the second, where `qout` is supposed to come from -- the `Wrapper` entry points, not the routers. 47 doctests pass, up from 46; `mkdocs build --strict` stays clean.
…d log line The SonarCloud sweep on PR 217. The quality gate passes on every condition, and two of its six CRITICAL findings are real and caused by this branch. `Catchment.extract_discharge` tipped over the cognitive-complexity limit (18 against 15) when this round added the UNROUTED refusal and the `qout` requirement. The cause was already there: the seven-metric block was written out twice, once per routing branch, which is also how the two would drift apart. `GAUGE_METRICS` names the seven once and `_score_gauge` fills a gauge's column from it, so both branches now say `_score_gauge(...)` and the method is back under the limit. `run.py` logged the literal "Model Run has finished" from three entry points; it is `RUN_FINISHED` now. Seven `pytest.raises` blocks in the tests this branch added wrapped more than one call that could throw -- `_optimization_args()`, `LumpedRun.from_model(...)`, `dt.datetime(...)` -- so a failure in the setup would have read as the refusal under test. The setup is hoisted out; only the call being tested is inside the block. The other four CRITICAL findings are false positives, reported rather than marked: `python:S5655` on `Run.run_flood`, `Run.run_distributed_with_lake` and `Calibration.run_calibration` claims the arguments are the wrong type. Those parameters are annotated with `typing.Protocol` classes (`CatchmentLike`, `SpatialDistribution`) that `Catchment` and `Parameters` satisfy structurally, which is the whole point of `hapi.protocols` -- and mypy checks all 28 modules clean.
|
This was referenced Sep 15, 2026
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.
Description
Rebuilds the coupling between
Catchment,Run,Wrapper,DistributedRRMandCalibration.Catchmentwas the only real object in the pipeline: 42 nullable attributes spanning configuration,inputs, results and presentation, which every other layer reached into by name.
Runsubclassed itpurely so a catchment could be passed where
selfwas expected — every entry point was calledunbound as
Run.RunHapi(model)— and the engines wrote nine result arrays back onto the object theyhad just read. That is content coupling, and it is why four modules were excused from mypy, why "has
this been validated?" was answered by remembering which entry point you came through, and why a
catchment was never in a knowable state between runs.
The shape now is:
Catchmentis a builder (its inputs are legitimatelyX | Noneuntil thematching
read_*call has run), andhapi.runsnarrows one into a validated run(
DistributedRun/LumpedRun) whose fields are not optional.DistributedRun.from_modelis thesingle validation seam, and the engine signatures take those types — so the check is enforced by the
signatures rather than by discipline. Results come back as a returned
SimulationResultscarryingthe routing that produced them.
Five value objects were extracted along the way:
SimulationPeriod,SimulationResults,ParameterSet/ConceptualModelSetup/ParameterBounds,RiverGeometry, and the protocols inhapi.protocols.Catchment.__init__drops from 42 attributes to 17; the engines read 2 attributesoff a catchment instead of 26, and write 2 instead of 9. Only
hapi.catchmentremains on the mypysuppression list, correctly, because it is the builder.
The presentation went with it.
plot_distributed_results,save_animationandsave_results— 332lines that take finished results and turn them into figures and files — now live on
SimulationResultsasanimate,save_animationandsave. They were the only reason a builderimported a plotting stack, and the only reason it held
animand_animation_glyph.catchment.pydrops from 1,760 to 1,428 lines. cleopatra is imported inside
animate, becausehapi.resultsiswhat every engine imports and a module-scope import would put matplotlib in the path of every model
run; a test holds it there.
plot_hydrographdeliberately stayed onCatchment— it reads no resultarray, it plots
Qsimagainst the observed gauge record.Nine latent bugs surfaced and are fixed here, most found by the refactor rather than by looking:
CalibrationbypassedRunand so skipped every validation it performed — on the one path thatrebuilds the parameter array once per trial vector, thousands of times per run.
except:in the objective functions reported every defect to the optimiser as a badparameter set. This had hidden five test failures: those tests were asserting on a stubbed
optimiser rather than on a run that had actually happened.
calibrate_maxbascalledWrapper.run_maxbas(run)withrunnever assigned.routing_method="Kinematic"crashedrun_distributedon abankfull_depthofNone.sums
quz_routedfrom upstream neighbours, its whole tributary vanished from the hydrograph.Lake.Qlake/Lake.QlakeRwere created by assignment inside the wrapper and declared nowhere.Calibration.parametersheld the optimiser's flat vector where a runnable(rows, cols, n)cubewas expected, leaving the model unrunnable after a calibration.
FlowNetworkpromised its two rasters share a grid but only checked at construction.Catchment.extract_discharge(only_outlet=)could never change which branch ran (issue refactor(catchment): decide what extract_discharge(only_outlet=) should do #209).One more, found by moving the code:
save_results' lumped branch built its date index with ahard-coded
freq="D", so an hourly lumped run wrote a daily index against hourly values.saveusesthe run's own calendar.
Two review rounds then found 48 more things, and the second round's main job turned out to be
auditing the first. Highlights, all reproduced before being fixed:
Run.run_lumped(model)— the entry point called with theRoute=0it declares as its own default— raised
Length of values (1096) does not match length of index (1095). Both routed branchestrimmed the conceptual model's unwritten trailing slot and the unrouted one did not.
trial infeasible, so it was swallowed one line later: a wrongly wired objective gave a full Harmony
Search over an all-
nanlandscape instead of an error. Arity is read off the signature before thesearch starts now.
read_objective_function's extra arguments were documented as forwarded and silently dropped bytwo of the three entry points.
_check_lake_meteorequired three columns while both lake wrappers readmeteo_data[:, 3].qoutwas a step longer on the Muskingum path than on the MAXBAS and lake paths, for a fielddocumented as one hydrograph.
route_maxbasandroute_maxbas_by_path_lengthnever recorded the routing they applied, soRoutingKind— presented throughout as a property of the arrays — was set only byWrapper.DistributedRun.from_modelchecked every input against the grid except the flow-path-lengthraster, which
route_maxbas_by_path_lengthindexes by it.tests/scripts called APIs that no longer exist; fourdocs pages documented removed constructors, a
Calibration.from_yamlthat never existed, and anobjective signature the code cannot call.
tests/sensitivity_analysis.pyreadUBfrom theLBfile andLBfrom theUBfile.Three of round 1's own fixes were wrong, and round 2 caught them. The worst made every
distributed calibration raise: a width check was added to
ParameterBounds, but those boundsdelimit the optimiser's flat search vector — 980 values on the Coello grid, 243 for HRUs — not the
12 parameters per cell the conceptual model reads. No width satisfied both that check and
par3d's.Nothing caught it because every distributed-calibration test hand-assigned a 12-wide bound. The rule
moved to
calibrate_lumped, the one path where the two are the same thing.Performance: the Muskingum routing rescanned the whole grid once per accumulation level —
O(n_acc x rows x cols), effectively quadratic, to visit each cell once.
FlowNetwork.cells_by_acc_valindexes the cells by accumulation code instead. Bit-identical output, verified by diffing against the
previous engine; 4,004 cell tests down to 89 on the Coello grid (9.86 ms to 2.18 ms), and the saving
grows with the grid: 2,500x at 50x50, 62,500x at 250x250.
No dependency changes.
Issues
extract_discharge(only_outlet=)could never select a branch; both it andframe_work_1are removed, and the path is chosen from the routing recorded on the results.Type of change
Check relevant points.
Breaking changes
Seven
refactor!commits, deliberately with no compatibility shims:Run.RunHapi/runFW1/runHAPIwithLake/RunFW1withLake/RunFloodModel/runLumpedRun.run_distributed/run_maxbas/run_distributed_with_lake/run_maxbas_with_lake/run_flood/run_lumpedWrapper.RRMModel/FW1/RRMWithlake/FW1Withlake/LumpedWrapper.run_muskingum/run_maxbas/run_muskingum_with_lake/run_maxbas_with_lake/run_lumpedDistributedRRM.SpatialRouting/DistMaxbas1/DistMaxbas2route_muskingum/route_maxbas/route_maxbas_by_path_length;Dist_HBV2deletedCalibration.FW1Calibration/lumpedCalibrationcalibrate_maxbas/calibrate_lumpedRun(Catchment),Run.from_yamlRunis a namespace of static entry points; build the model and pass it inCalibration(name, start, end, ...)Calibration(Catchment(...)); everything inherited is reached through.modelmodel.qout/.Qtot/.quz/.qlz/.quz_routed/.qlz_translated/.state_variablesmodel.results.*, andQtotis nowq_totalmodel.start/.end/.date_index/.dt/.conversion_factor/.temporal_resolutionmodel.period.*model.parameters(array),.snow,.maxbasmodel.parameters.values/.snow/.maxbasmodel.lumped_model/.area/.initial_cond/.q_initmodel.model_setup.*model.DEM/.bankfull_depth/.river_width/.river_roughness/.flood_plain_roughnessmodel.river_geometry.*model.LB/.UB,Catchment.read_parameters_boundCalibration.bounds.lower/.upper,Calibration.read_parameters_boundWrapper.*andDistributedRRM.*taking aCatchmentDistributedRun/LumpedRun; build one withDistributedRun.from_model(model)extract_discharge(frame_work_1=, only_outlet=)model.plot_distributed_results(start, end, option=1, gauges=True)model.results.animate(start, end, option=1, gauges=model.GaugesTable)—gaugestakes the table, not aboolmodel.save_animation(path, fps=2),model.animmodel.results.save_animation(path, fps=2),model.results.animmodel.save_results(flow_acc_path, result, start, end, path, prefix)model.results.save(path, result=, start=, end=, prefix=, flow_acc_path=)—pathis now the first positional argumenthapi.catchment.STATE_VARIABLEShapi.results.STATE_VARIABLESMeteoDataof 3 columnsmeteo_data[:, 3], the long-term average, so a 3-column record passed validation and then raisedIndexErrorinside the runresults.qoutwaslen(period) + 1after a Muskingumextract_dischargelen(period)on every path, matching MAXBAS and the lake routesnanObjectiveFunctionArityErrorbefore the search startsrun_calibration/calibrate_maxbasignoredread_objective_function's extra argumentsHow Has This Been Tested?
Run from the repo root:
export HAPI_DATA_DIR=src/hapi/parameters pixi run -e dev main pixi run -e dev plot pixi run -e dev mypy pixi run -e dev doctests pixi run -e dev pre-commit run --all-filescontext each time. Round 1 raised 24 findings, round 2 raised 24 more; all 48 are resolved,
one commit per finding. Round 2's value was mostly in auditing round 1: three of round 1's
own fixes were wrong, including a
ParameterBoundswidth check that made every distributedcalibration raise (the bounds delimit the optimiser's 980-value search vector, not the model's
12 parameters per cell). The review files are
pr-diff-review-*.md.plottask. 58 netnew test functions, covering the narrowing seam, the result object, the routing provenance, the
bucketed index, the
Run/Calibrationcoupling and the moved presentation methods.results,runs,period,conceptual,distrrm,run,wrapper, andcalibrationat 97% (up from 87%, the one module the branchhad never measured — which is why round 2 found three wrong fixes there).
tests/rrm/catchment/test_results.pypinswhat the moved methods refuse: a lumped run asked to animate, a routed field read before its
routing step filled it, a date that is not a step of the run, rasters with no georeferencing
template, and the lumped CSV options nothing else exercised.
run,wrapper,distrrmandcalibrationtakenoff the suppression list (only
hapi.catchment, the builder, remains).period,results,conceptual,runsandcalibrationadded to thetask.
SimulationResults.animate,save_animationandsaveeach carry a runnable example;save's builds a smallLumpedRunand writes a real CSV.alongside the new one and comparing
quz_routed,qlz_translatedandq_totalwithnp.array_equalover the Coello example, rather than relying on the tests alone.examples/hydrological-model/coello/run/coello-flood-model-run.py,restored from Serapis where it had been left behind and could no longer run against this package.
hapi.run,hapi.wrapperandhapi.rrm.distrrmin a subprocess pulls in neither matplotlib nor cleopatra, even thoughSimulationResultsnow renders itself.mkdocs build --strictis clean, down from 30 griffe warnings. Those weredocstrings describing signatures that no longer exist:
Wrapper's five entry points stilldocumented
Model,ll_temp,q_0andskip_hydraulic_cells, removed when the enginesstarted taking a
DistributedRun. The newdocs/api/results.mdpage renders.were real and caused by this branch and are fixed:
extract_dischargehad tipped over thecognitive-complexity limit because the seven-metric block was written out once per routing
branch (now
GAUGE_METRICS+_score_gauge, which removes the duplication too), and a logliteral repeated three times is now a constant. Seven
pytest.raisesblocks that wrapped morethan one throwing call were tightened. The remaining four CRITICALs are
python:S5655claimingwrong argument types on
run_flood,run_distributed_with_lakeandrun_calibration— thoseparameters are
typing.Protocolannotations thatCatchmentandParameterssatisfystructurally, which is what
hapi.protocolsis for, and mypy checks all 28 modules clean.Reported rather than marked: accepting a finding is the maintainer's call.
Not tested: the 11 notebooks touched by the renames were not executed. 12 notebooks still import
the pre-rename
Hapipackage and do not run today, which thenotebookstask already records — theedits keep them internally consistent but do not revive them. That migration is separate work.
Checklist:
The three version items are left unticked on purpose. The breaking changes imply a major bump, which
is
cz-bump's job: it writesdocs/change-log.md(this repo has noHistory.rst) and updatespyproject.tomlfrom the commit messages, so doing it by hand here would conflict with that.Documentation: docstrings throughout, plus
README.md,docs/api/catchment.md, the newdocs/api/results.mdand thedocs/examples/*.mdpages updated for the renames.