feat(config): build a Catchment from a YAML run configuration - #214
Merged
Conversation
Standalone script replaying the workflow behind test_e2e_coello_from_netcdf.py::TestMuskingumPipeline:: test_the_drivers_come_from_the_file_and_cover_the_model: one MeteoInputs.from_netcdf call replaces the three raster-folder reads, through the routed fields, per-gauge metrics and the saved rasters.
Every example run script opened with a block of hardcoded path and constant assignments, then called the `read_*` methods in the exact order the build-then-mutate pattern requires. Configuring a case study meant editing Python, and the ordering constraint was rediscovered per script. `hapi.config` lifts that block into a YAML file: `load_config` parses it into a `RunConfig` of dataclasses, and `from_yaml` reads every input and assigns it onto the model in the required order. `Catchment.from_yaml` exposes the same thing as an alternate constructor, delegating through a local import because `hapi.config` imports `Catchment` to build one. Running the model stays the caller's job, as in a hand-wired script. The schema covers lumped and distributed runs, which disagree on the shape of two blocks: `meteo` is a grid for distributed and a single CSV for lumped, and `gauges.discharge` is a folder of per-gauge CSVs against a single file. A name-to-class registry resolves `conceptual_model.model_class`, since the model is a class rather than data. `catchment.routing_method` is canonicalised to the exact literal the internals compare against: `Catchment.__init__` stores it verbatim, and `distrrm.SpatialRouting` tests `!= "Muskingum"` case-sensitively, so a lowercase spelling silently sent every cell down the MAXBAS branch and read `bankfull_depth`, which is None outside the flood model. Also fix three methods whose `str | dt.datetime` parameters called `strptime` unconditionally, so passing the documented `datetime` raised TypeError: `plot_hydrograph`, `read_discharge_gauges` (the `split` path) and `save_results`. Each now branches on `isinstance(..., str)`. - add `pyyaml` as a dependency; it was only present transitively - port coello-distributed-model-run-netcdf.py onto `Catchment.from_yaml`
MAfarrag
changed the title
YAML plain scalars admit interior spaces, so `results/saved rasters/` parses to the same string quoted or not. The quotes elsewhere in the file are load bearing and stay: an unquoted `2009-01-01` becomes a `date` rather than the `str` the config expects, and an unquoted `%Y-%m-%d` is a scanner error, `%` being a reserved directive indicator.
Ports the remaining three Coello run scripts onto `Catchment.from_yaml`, each with its config beside it, so all four now configure a case study by editing data rather than Python. These cover the schema's variant-specific shapes, which the NetCDF example did not exercise: the lumped pair reads one CSV of catchment-average drivers instead of a grid and one discharge file instead of a gauge table plus folder, and the distributed MAXBAS run loads no flow-direction raster, since triangular routing sends every cell straight to the outlet. Routing stays in the scripts. Which function routes a lumped run is a run-time choice handed to `Run.runLumped`, not an input, so `Routing.muskingum_v` and `Routing.triangular_routing_1` are still picked there. Verified each config assembles a model field-for-field identical to the hardcoded block it replaces -- parameters, drivers, gauges, flow network and the rest -- and that all three scripts still run to completion.
…atchment `hapi.config` imported `Catchment` in order to build one, so `hapi.catchment` could only reach back through an import inside the method body. The cycle was the design being wrong, not something to work around: a module that parses configuration has no business constructing models. `hapi.config` now describes data and nothing else, and imports nothing from `hapi`. Being a leaf, it is imported normally at the top of `hapi.catchment` and the local import is gone. Everything that assigns to a model -- the construction, the `MeteoInputs` / `FlowNetwork` loaders, the `read_*` order, the conceptual-model registry and the routing-label translation -- now lives in `Catchment.from_yaml`, which also drops `load_config` in favour of validating the parsed mapping where it is used. The blocks are pydantic models rather than dataclasses, which moves the rules that were assertions in the builder into the schema, where a configuration can be rejected before anything is read: - `Literal` types on `spatial_resolution`, `temporal_resolution`, `routing_method` and `meteo.source` name the accepted values in the error - `extra="forbid"` catches a misspelled key instead of dropping it - `initial_condition` must hold five states, `catchment_area` must be positive - a model validator enforces the fields each spatial resolution needs: `flow_network` and `gauges.table` for distributed, `meteo.path` for lumped, and `meteo.path` again for `source: netcdf` `from_yaml` builds `cls`, so `Run.from_yaml` and `Calibration.from_yaml` return their own type. `hapi.config.load_config` and the module-level `hapi.config.from_yaml` are gone; build a model with `Catchment.from_yaml(path)`, or validate a configuration on its own with `RunConfig.model_validate(...)`. Not a breaking change: both were added earlier on this same branch and never reached main.
…tion
The dispatch over `meteo.source` was a private helper in `hapi.catchment`, yet
every line of it reached into `MeteoInputs` to choose between that class's own
three loaders. It belongs on the class it envies, next to the loaders it picks
from, where all four ways to construct the drivers can be read together.
`hapi.config` is a leaf, so `hapi.inputs` importing it adds no cycle:
config -> nothing catchment -> config, inputs, rrm.hbv*
dem -> nothing run -> catchment, wrapper
inputs -> config, dem
`from_config` takes the fallback window as plain `start` / `end` rather than a
`CatchmentConfig`, so `MeteoInputs` needs to know nothing about the catchment
block. Assigning the result stays in `Catchment.from_yaml`.
`hapi.inputs` is not in the mypy suppression list that covers `hapi.catchment`,
so the driver fields being optional -- a lumped configuration sets none of them
-- now has to be narrowed rather than ignored. The asserts document that
`RunConfig` already guarantees them, and only fire for a `MeteoConfig` built by
hand.
`FlowNetwork` is deliberately left alone: its block maps to a single
`from_rasters` call, so a `from_config` there would be an alias with no
dispatch to encapsulate.
`assert` is stripped under `python -O`, so the guards on the driver fields were not validation at all: an incomplete `MeteoConfig` would have fallen through to the loader and failed further in, on a `None` path. Each now raises `ValueError`. The three drivers are checked together so the message names which of them the configuration leaves unset, rather than reporting only whichever assert happened to fire first, and `meteo.path` gets its own message explaining why `source: netcdf` needs it. The three are bound to locals before the check: a comprehension over `METEO_VARIABLES` reads better but mypy cannot narrow through it, and `hapi.inputs` is not in the suppression list that covers `hapi.catchment`. Verified under both `python` and `python -O` that the same `ValueError` reaches the caller.
`assert` is stripped under `python -O`, so seven checks in this module were not validation at all. Each now raises the exception that fits, and the three `Raises:` entries advertising `AssertionError` are corrected: - `read_lumped_model`: `q_init` and `initial_cond` type checks -> `TypeError`, naming the type that arrived - `read_parameters_bound`: unequal bound lengths -> `ValueError`, naming both - `read_discharge_gauges`: the gauge table not read yet -> `ValueError` - `save_results`: an out-of-range lumped `result` -> `ValueError`, matching what the distributed branch already raises - `Lake.__init__`: an unknown `temporal_resolution` -> `ValueError`, matching the wording `Catchment.__init__` uses - `Lake.read_lumped_model`: the `initial_condition` type check -> `TypeError` The gauge-table guard was dead as written: `__init__` sets `GaugesTable` to None, so `hasattr(self, "GaugesTable")` was always true and a caller who skipped `read_gauge_table` fell through to a `TypeError` on None a few lines later. It now tests for None and says which call is missing. `test_a_non_float_initial_discharge_is_refused` expected the `AssertionError` and now expects the `TypeError`. Verified under both `python` and `python -O` that all of them still raise.
`assert` is stripped under `python -O`, so the nineteen dimension checks guarding the distributed entry points were not validation. Each now raises `ValueError`, and the five `Raises:` entries advertising `AssertionError` are corrected. Most of them were the same checks copied across the five entry points, so the repeated ones move into two helpers rather than becoming nineteen `if` blocks: `_check_parameters_cover_grid` for the parameter rows and columns, which every distributed method asserted, and `_check_lake_meteo` for the length and column count of a lake record, asserted by both lake-aware methods. The rest -- the flow-direction grid check and the flood model's four river-geometry arrays -- are converted where they stand. Messages are unchanged, since the tests match on them. The one exception is `RunFW1withLake`, which said "three columns rain" where `runHAPIwithLake` said "three columns of rain"; sharing a helper settles it on the latter, and the test matches "three columns" either way. `test_run_validation` expected the `AssertionError` in five places and now expects the `ValueError`. Verified under both `python` and `python -O` that the checks still fire.
Completes the sweep: `src/hapi` now contains no `assert`, so no input check disappears under `python -O`. Twenty-three across seven modules become the exception that fits -- `TypeError` where a type is wrong, `ValueError` for a value or a length -- and the fourteen `Raises:` entries advertising `AssertionError` are corrected, including the one on the abstract `routing` in `base_model`, which declares the contract its implementations keep. Three messages were wrong where they stood and are rewritten rather than carried over: - `run_calibration` and `FW1Calibration` guarded `type(api_obj_args) is dict` with "store_history should be 0 or 1", and the solver arguments with "history_fname should be of type string". Both now say which bundle is not a dict and what arrived instead. - `lumpedCalibration` checked `basic_inputs` for 'Route' and 'RoutingFn' while reporting "should contain ['p2','init_st','UB','LB']". It now names the keys it looks for and which are missing. - The four `maxbas` checks test `>= 1` but read "has to be larger than 1". The dict checks appeared three times over, so they move into `_check_optimization_args`; that also settles `type(x) is dict` against the `isinstance` the third copy already used. Verified under both `python` and `python -O` that the checks still fire, and that the eighteen failing doctests in this package are exactly the eighteen that failed before -- the pre-existing set the doctest hook is disabled for.
MAfarrag
force-pushed
the
feat/input-yaml-file
branch
from
August 30, 2026 14:43
db97eeb to
1b4c6b6
Compare
`from_yaml` assumed every model reads a fitted parameter set and is scored against gauges, but neither holds generally: - a calibration derives its parameters from the bounds handed to `read_parameters_bound`, so it never calls `read_parameters` and has no parameter path to name - a run that is not scored against observations has no gauge data at all Both blocks are now optional, and the builder skips the corresponding `read_*` call when one is absent. The distributed check on `gauges.table` applies only when the block is present, since it exists to catch a gauge table missing from a configuration that does carry gauges.
`hapi.config` and `Catchment.from_yaml` arrived on this branch with no unit tests: the schema sat at 72% and the builder was not exercised at all, its happy path covered only by a doctest. Forty tests, one class per block plus one for the builder. The emphasis is on the cross-field rules, since which blocks a configuration needs depends on `spatial_resolution` and `meteo.source` -- each is pinned separately, with the message it produces, so a rule that stops firing says which one. The builder is covered on both shapes: the distributed path down to every populated attribute, and the lumped path, which reads one averaged-driver CSV and skips the gauge table it has no grid to locate gauges on. `hapi.config` goes 72% -> 100% line and branch. The suite also caught a false claim in the `from_yaml` docstring. It said `Run.from_yaml` returns a `Run`, but `Run` overrides `__init__` to take only `self` -- its entry points are called unbound on a catchment -- so the call raises `TypeError` at the constructor. The docstring now says so, and a test pins the behaviour rather than the wish.
`flow_direction` is optional on the block because MAXBAS sends every cell straight to the outlet and never reads one. Nothing tied that to the routing method, so a distributed Muskingum config without it validated, built, and then died inside `Run.RunHapi` on `flow_dir_arr.shape` -- a bare `AttributeError` naming neither the block nor the file, raised after the meteo cubes, the accumulation raster and the whole parameter folder had been read. `muskingum` is the default, so the likeliest way in is copying the shipped MAXBAS example's `flow_network` block, which legitimately omits the raster. The validator now states the rule, with a test on each side of it.
`Path(path).read_text()` uses the locale codec, so a configuration carrying a non-ASCII catchment name or path decoded to different text on a machine whose default is not UTF-8 -- silently, since the mojibaked result is still valid YAML. The name reaches result filenames and plot titles; a corrupted path produces a FileNotFoundError naming something the user cannot find in their own file. Version-dependent, so it passed on 3.14 and failed on the 3.11 floor the project declares and CI runs. The new test is green on both.
`__init__` case-folded `spatial_resolution` and `temporal_resolution` but stored `routing_method` verbatim, while `distrrm.SpatialRouting` compares `routing_method != "Muskingum"` case-sensitively and its false branch reads `bankfull_depth` -- None outside the flood model. A lower-case "muskingum" therefore routed every cell down the MAXBAS branch and raised `TypeError: 'NoneType' object is not subscriptable`. The branch had been papering over this in `from_yaml` alone, which left the bug live on the hand-written path every example and downstream script uses, and made `Catchment.routing_method` mean different things depending on how the object was built. The constructor now validates and canonicalises like the other two enumerated arguments, and `_ROUTING_METHOD_LABELS` is gone. "Kinematic" is in the accepted set: that same `!= "Muskingum"` comparison is how `Run.RunFloodModel` selects its path, so rejecting it would have broken the flood model.
When the `meteo` block states no window it inherits the catchment's dates, which are written in `catchment.fmt` -- but `from_config` parsed them with `meteo.fmt`. The two are independent fields with independent defaults. The loud case merely confuses, blaming a date the user never wrote that way. The quiet one is worse: between two mutually parseable layouts such as "%d-%m-%Y" and "%m-%d-%Y" the drivers are windowed to the wrong period, and since `MeteoInputs` pairs with `date_index` by position the model then runs on drivers offset from its own calendar with no error at all. `from_config` now takes the inherited format alongside the inherited dates, resolves each bound with the format it belongs to, and passes datetimes on so no loader can re-parse them. The bound annotations widen to match `_as_datetime`, which has always accepted a datetime; `read_rasters` rejects one explicitly on the `date=False` path, where bounds are indices rather than dates and `int()` would have failed several frames down.
A `routing_method: maxbas` run pointed at a 12-parameter Muskingum set validated, built and ran to completion, with `DistMaxbas2` reading the Muskingum X as the MAXBAS value -- a hydrograph that is quietly wrong, which is the worst failure mode for a modelling tool. The reverse pairing is symmetric: `Run.RunHapi` reads K and X out of an 11-parameter MAXBAS set. The parameter-count check cannot catch either. A MAXBAS set holds 11 parameters and a Muskingum set 12, and `parameters.maxbas` is what selects which count is expected, so a disagreeing pair still counts correctly. `RunConfig` now requires the two to agree whenever a `parameters` block is present. A calibration declares none -- its parameters come from the bounds -- so nothing there is constrained. The check is scoped to distributed runs: a lumped run picks its routing function at call time, not from `routing_method`.
`outputs.results_dir` was parsed, shipped in an example, and read by nothing: `from_yaml` discarded the `RunConfig` after building, so no caller could reach it even in principle. The example that set it then hardcoded the same path in Python two cells later, teaching that editing the field has an effect it did not have. `from_yaml` now leaves the validated configuration on `model.config`, so the blocks the build does not itself consume stay reachable. The netcdf example reads both its output directory and its flow-accumulation path from there instead of restating them -- the latter answering the wart its own comment admitted, since `FlowNetwork` keeps the arrays but not the source path. The example also writes beside the other example outputs rather than into the repository root, and states its driver/model checks with `raise` rather than `assert`, which a branch about asserts not surviving `python -O` should not have shipped.
The assert-to-raise conversion split some checks across two exception types but left four `Raises:` sections describing the old single one. Since the point of the conversion is that callers can now catch these by type, those sections are the contract and were wrong: the calibration entry points list `ValueError` for optimization arguments that `_check_optimization_args` rejects with `TypeError`, and `Parameters.__init__` lists `TypeError` for a `lumped_par_pos` length mismatch that raises `ValueError`.
`assert maxbas >= 1` fired for NaN, since the comparison is false. Rewriting it as `if maxbas < 1` inverted that: NaN is not less than 1 either, so the guard stopped firing and execution reached `int(round(nan, 0))`, raising "cannot convert float NaN to integer" -- a message naming neither maxbas nor the routine it came from. A calibrated MAXBAS value can legitimately be NaN in a masked cell, so the guard is written as `not maxbas >= 1` at all four sites, which preserves the original semantics exactly.
…t use Two gaps in the promise that a validated configuration is consumable without re-checking. Dates were never checked against the format they are written in, so `start: not-a-date` validated and failed later inside `Catchment.__init__`; nothing checked that a period runs forwards either, so a reversed one produced an empty date index and failed downstream on a shape mismatch. Both are now caught per field, named, and the period order with them. A lumped configuration could also carry a `flow_network` block or a grid `meteo.source`, both silently discarded -- the block simply never took effect, and a `source: netcdf` lumped run would hand a `.nc` path to `pd.read_csv`. `extra="forbid"` is used precisely so a misspelled key fails rather than being dropped, so accepting a correctly spelled but inapplicable one was the same silence by another route.
…input The registry lookup sat below `read_parameters`, so a typo'd `model_class` cost the whole parameter folder read before failing and left a partly populated model behind. It needs nothing but the config, so it now runs before the first reader -- the same reasoning the diff's own test states for validating the mapping up front.
…olumn truthfully `from_netcdf_files` takes a `variable` for files holding more than one, but `MeteoConfig` had no field for it and `from_config` never passed one, so such a user hit "pass variable= to pick one" with no way to comply from YAML. `GaugesConfig.column` claimed to select the discharge file names. It does not: `read_discharge_gauges` reads `<id>.csv` regardless and only labels the resulting frame with `column`, so anything but "id" produces a frame whose declared columns are never written. The docstring now says what it does. `ConceptualModelConfig` also re-spelled `extra="forbid"` rather than extending `_STRICT`, which would have let a future change to the shared config skip that one class.
`from_yaml` annotated `path: str` while immediately wrapping it in `Path`, so a caller holding a `Path` had to stringify it -- and the rest of the package annotates such arguments `str | Path`. Its `Raises:` also listed only the validation errors, omitting the two most likely for the intended audience: a missing file and malformed YAML. An empty file was the worst of them, parsing to None and surfacing as pydantic's "Input should be a valid dictionary" without naming which file was empty. It now raises saying so.
Every test and both doctests used `source: netcdf`, leaving the other two dispatch branches and both defensive raises unexecuted. The gap mattered most for `rasters`: it is what the shipped MAXBAS example depends on, and it forwards seven keyword arguments plus two conditional ones, the exact shape of code where a mis-forwarded argument stays invisible until a user's file names stop parsing. Five tests over the bundled Coello fixtures: the raster branch against the three folders, the conditional pass-throughs, the per-driver NetCDF branch, and the two raises. `hapi.config` holds at 100% line and branch.
… run `hapi.config` is not an internal helper -- it is the user-facing file format, and its docstrings are the only description of the schema -- but it had no API page and no nav entry, so the reference was unreachable from the rendered site. Adds `docs/api/config.md` covering `RunConfig` and each block it nests, with a pointer to `Catchment.from_yaml`. The new doctests also read paths under `tests/` and `examples/`, which are not in the wheel. They stay executable, since that is what keeps them honest, but each Examples section now says the paths are repository fixtures rather than implying an installed user can run them as written.
`read_discharge_gauges`, `plot_hydrograph` and `save_results` gained `isinstance(x, str)` guards so a caller can pass the `datetime` their `str | dt.datetime` annotations already promised, but their docstrings still described the arguments as `str` only. Three public signatures widened without saying so; they now say so.
Two lines added by the round's fixes had no test: the guard rejecting a datetime bound when `read_rasters` is ordering by index rather than date, and the constructor's `spatial_resolution` check, whose routing-method sibling was already pinned. `hapi.config` holds at 100% line and branch, and `from_yaml` and the constructor now have no uncovered line between them. What remains uncovered in `catchment.py` and `inputs.py` is pre-existing surface this branch does not touch -- the plotting and animation methods, and the `Inputs` parameter helpers.
`spatial_resolution`, `temporal_resolution` and `routing_method` are all lower-cased in the constructor, so a non-string reached `.lower()` and raised an `AttributeError` naming neither the argument nor the class. `Calibration` made that reachable through its own signature, which declared all three `str | None` while passing them straight down -- an annotation advertising an input that crashes. The three are now checked together before any of them is lower-cased, raising a `TypeError` that names the argument and what it got, and `Calibration`'s annotations drop the `| None` they could never honour.
`ROUTING_METHODS` accepts three methods and `CatchmentConfig.routing_method` names two, with nothing at either site saying the other exists. The gap is deliberate -- `kinematic` selects the flood model, whose inputs the schema does not carry -- but a reader hitting "Input should be 'muskingum' or 'maxbas'" on a value the constructor takes has no way to learn that from the code. Each site now points at the other and states the rule: the schema says why the third is unreachable, and the registry says that adding a method to it needs a decision in the schema.
The NaN-maxbas hardening reached `triangular_routing_2` and the three conceptual models but not `calculate_weights`, which is where `triangular_routing_1` -- the function the lumped MAXBAS example and `DistMaxbas1` actually route with -- resolves its weights. Below one whole step the triangle has nothing to spread over: 0.5 produced a single weight and an all-zero hydrograph with nothing raised, 0 an `IndexError` about axis 1, and NaN "cannot convert float NaN to integer". The guard uses the same negated `not maxbas >= 1` form as the others, so NaN still fails here naming the parameter. It immediately caught a real one. The lake tests built their model from the Muskingum parameter set and then routed it triangularly, and `DistMaxbas1` reads `parameters[..., -1]` -- MAXBAS in a MAXBAS set, the Muskingum X in that one. Every cell routed with X = 0.2 and returned zeros, so four tests asserted their shapes and flags against output carrying nothing. They now build from the MAXBAS set, whose in-domain values run 1.4 to 2.4. The one test that drives both paths on a single model re-reads the Muskingum set for its Muskingum leg, since the two index different bands; it is still the same model object, which is what its flag assertion is about.
`save_results` built its raster names with `path + prefix + date`, so a directory written the normal way -- without a trailing separator -- produced `some/dirResult_2009-01-01.tif` beside the directory rather than inside it. The directory was also never created, and `path` was dereferenced with no type check even though `outputs.results_dir` is optional in a run configuration, so a caller forwarding it straight through hit `TypeError: unsupported operand type(s)` from a string concatenation rather than anything naming the argument. The names are now joined, the directory is created when it is missing, and a non-string `path` raises a `TypeError` that says what the argument means in each mode -- a directory when distributed, the CSV itself when lumped, which the docstring now states too. The NetCDF example stops assuming the optional `outputs` block is there and falls back to the working directory.
The four YAML-driven examples write into the same data tree they read from, so their results land beside tracked inputs in directories `.gitignore` did not cover -- ten `Result_*.tif` under the distributed model's results folder and four `*Results-Lumped-Model_*.txt` under the lumped one, all showing as untracked in every `git status` after a run. The inputs stay tracked; only the per-run artefacts are ignored.
`model_class` was resolved ahead of the readers because a typo there would otherwise cost the whole parameter folder read before failing. The same argument applies to every path in the file, and none of them were checked: a misspelled gauge table was reported only after the meteorological cube and the parameter rasters had been read, which on a real grid is minutes to learn about one line. `from_yaml` now checks the paths the chosen shape will open -- the same set the schema validated against -- and raises one `FileNotFoundError` listing all of them with the field that named each. A configuration with two typos reports both at once, in hundredths of a second. Under `source="netcdf"` the three driver fields name variables inside the file rather than paths, so only the file itself is checked.
… pairs The date check compared `catchment.start`/`end` and `meteo.start`/`end` as two independent pairs and skipped a pair when either half was unset. But `MeteoInputs.from_config` mixes them -- each bound comes from `meteo` when stated and falls back to `catchment` otherwise -- so a `meteo` block stating only a start after the catchment's end gave an inverted effective window with neither pair inverted, and validated. The eventual failure was informative, but it is precisely the case this validator exists to catch. The window the run will actually use is now resolved the same way `from_config` resolves it and checked once, and the message says where each bound came from. A `meteo` window that is a genuine sub-period of the catchment is unaffected.
`Run` subclasses `Catchment` to hold its entry points, not to be a catchment, so the inherited `from_yaml` was a discoverable public classmethod whose only behaviour was to fail on constructor arity -- "__init__() got an unexpected keyword argument 'fmt'", which says nothing about `Run` being an unbound-method holder or about what to call instead. The override raises the same `TypeError` with a message that does: build the model with `Catchment.from_yaml` and pass it to `Run.RunHapi(model)`.
`pydantic >=2.0` was open-ended, so a pydantic 3 release would be resolved into every new environment. `hapi.config` is written against pydantic 2's API -- `ConfigDict` spreading, `protected_namespaces=()`, `model_fields_set`, `Field(min_length=...)` on a list -- none of which a major release is obliged to keep, and the schema is the one place a silent behaviour change would be hardest to notice. The resolved version is unchanged, so no lockfile update follows.
`tests/test_config.py` sat at the tests root while depending on `coello_acc_path` and `coello_dist_parameters_muskingum` from `tests/rrm/conftest.py` and on `lumped_parameters_path`, `lumped_meteo_data_path` and `lumped_gauges_path` from `tests/rrm/catchment/conftest.py`. It worked only through the `from tests.rrm.<sub>.conftest import *` re-export chain, which inverts the direction that structure is meant to run in: a file at the root reaching down into the deepest conftest rather than a file beside it using what is already in scope. Moved to `tests/rrm/catchment/test_config.py`, where every fixture it uses is local or inherited normally. No test body changes.
`pixi` re-resolved after the `<3` bound was added. The only change is the recorded constraint; no package version moves.
Three pieces of prose had drifted from what the code does: The module docstring said a configuration that validates can be consumed "without re-checking", which read as a guarantee where it is an aspiration -- the builder still resolves `model_class` against its registry and still has to find the files. It now names both, and says where they happen. The out-of-scope list named only `Lake` and `RunFloodModel`. Also unreachable from the schema: `read_flow_path_length` and therefore `DistMaxbas2`, `read_river_geometry`, and reading a driver folder by numeric file order -- which is only reachable through `per_variable`, where it now meets the catchment window the schema always passes down and `read_rasters` refuses the pair. The `cls(...)` call in `from_yaml` passes its first three arguments positionally because `Catchment.__init__` names the second `start_data` and `Calibration.__init__` names it `start`. Tidying them into keywords would break `Calibration.from_yaml`, which the docstring advertises, so the constraint is now written down beside it.
`MeteoInputs.from_config` re-checks two rules `RunConfig` already enforces, which is right -- a `MeteoConfig` built by hand never passed through the schema -- but each site wrote its own sentence, and they had already drifted: "a distributed run needs all three drivers; meteo is missing ..." against "MeteoInputs needs all three drivers; the configuration leaves ... unset", and two different phrasings of the NetCDF path rule. Both messages now live in `hapi.config`, which `inputs` already imports, so the same rule reads the same way wherever it fires. The tests that pinned the old wording follow.
The new `from_yaml` and `from_config` examples carry non-skipped doctests that read real data and assert on concrete values -- shapes, step counts, a canonical routing method. Nothing ran them: the whole-`src` doctest hook is commented out because 19 examples in the HBV modules are stale, and no task or CI step covered the rest. They would have drifted the first time the fixtures or a reader default changed, invisibly. A `doctests` pixi task runs the four modules whose examples pass -- config, catchment, inputs, routing: 24 examples, all green -- with a matching pre-commit hook and a step in the lint workflow's `static` job, which is the one with the dev environment. The whole-`src` hook stays disabled; widening the task is how the remaining modules get added as their examples are repaired.
The task's `cmd` names four module paths on one line, which ran to 138 characters against the repository's 120-character limit. TOML's array form takes the same argument vector across four lines.
The branch converted all four example scripts to `Catchment.from_yaml` but left the two documentation pages that mirror them teaching the hand-wired flow, and the only mention of the feature anywhere in `docs/` was the generated API stub. A reader learning Hapi from the documentation would not have found it, and the docs page and the example script for the same Coello lumped run no longer agreed on how a model is assembled. A new Examples page walks through one configuration end to end: a complete lumped file, what changes for a distributed run, the three `meteo.source` loaders, why paths resolve against the file, what the schema checks before anything is opened, and what stays out of scope. The two existing run pages link to it from the top, so both routes are discoverable from either.
Every behaviour changed in round two now has a test, and `hapi.config` reaches 100% line and branch coverage (was 94%). Schema: the unquoted-YAML-date normaliser over a date, a timestamp and a custom format, plus the two inputs it defers to pydantic on; each inapplicable field a lumped run and each `meteo.source` now refuse, and the counterpart that a default the author never wrote is not held against them; `routing_method` derived from `parameters.maxbas` both ways, the agreement check now running for lumped too, and the ordering that lets a derived MAXBAS run omit the flow-direction raster; the resolved meteorological window in its three inverted shapes and one valid sub-period; `gauges.table_fmt` falling back. Builder: the three documented `from_yaml` raises that had no test -- a missing file, malformed YAML, and a top-level scalar; the pre-flight check naming two missing paths at once, and not checking a NetCDF variable name as a path; the raster source, which every other `from_yaml` test skips by driving from one combined NetCDF; `gauges.column` labelling the frame with no column left NaN; the three constructor mode-argument `TypeError`s, through `Calibration` as well. Elsewhere: `save_results` joining a directory written without a separator, creating a nested one, and refusing a non-string path; the `calculate_weights` lower bound over 0.5, 0 and NaN, reaching `triangular_routing_1`, with the boundary value still routing; and a direct success test for `MeteoInputs.from_config` under `source="netcdf"`, which was covered only transitively.
Of the public symbols this round touched, `Run.from_yaml` and `config.missing_drivers_message` were the two carrying no Examples section. Both now have one, and both run: the refusal example prints the first clause of the message it raises, and the one beside it builds a model from a shipped configuration and routes it, which is the pattern the refusal is pointing at. The message helper shows the singular and plural forms it chooses between. `run.py` joins the `doctests` task now that its examples execute -- 26 passing across the five modules.
The pre-commit job failed on the round-2 commits: ruff-format wraps at 88 characters, which is narrower than the 120 the prose limit allows, so several test signatures and one `parametrize` call needed breaking. Three files had also picked up mixed line endings. Formatting only -- the suite and the doctests are unchanged at 546 and 26.
`read_lumped_model` measured `initial_condition` before typing it, so `None` reported "object of type 'NoneType' has no len()" instead of naming the argument -- and the type check that followed carried an `is not None` that could never be false, because `len` would already have raised (S2589). Typed first, then measured; a non-list is refused the same way it was, with the message it had. `lumpedCalibration` indexed `initial_values[i]` over `range(len(self.LB))` behind a guard that only checked the list was non-empty, so a list shorter than the bounds indexed out of range part-way through building the optimisation problem, naming neither argument and leaving `opt_prob` half-populated (S6466). The lengths are now compared up front and the mismatch is reported with both counts.
Two functions were over the cognitive-complexity gate, and both for the same reason: a single body holding two branches that share nothing. `RunConfig`'s block validator was at 23 -- it grew this round with the inapplicable-field checks -- and `read_discharge_gauges` at 19. Each now dispatches on the resolution and nothing else, with the distributed and lumped bodies in methods of their own. A distributed run describes a grid, a routing network and a folder of per-gauge files; a lumped one describes two CSVs. Reading either no longer means skipping past the other, and the docstrings can say what each branch actually requires rather than covering both at once. No behaviour change: same checks, same messages, same order.
Eight `pytest.raises` blocks built their input inside the block -- `write_yaml` five times, a hand-built `MeteoConfig` twice, a `datetime` once -- so a failure in the setup would have been caught and read as the behaviour under test (S5778). Each is now built above the block, which also names it. Also: the assertion that the pre-flight check reports both missing paths is split in two, so a failure says which one was absent (S9073); the two fixtures drop the `scope="function"` that is already pytest's default (S9117); and the lumped example builds its scores dict as a literal (S7498).
Three calibration entry points each wrote their own loop adding one bounded variable per parameter, and the lumped one wrote it twice -- once seeded with a starting point and once not. Folding the length check into that duplicated pair is what pushed `lumpedCalibration` over the cognitive-complexity gate (S3776). One helper now declares the variables for all three, taking the optional starting point, so the bounds-length check lives beside the loop it protects rather than beside one of four copies. Also hoists the last `write_yaml` out of a `pytest.raises` block (S5778).
…added The pre-merge check's diff-coverage pass found three untested branches, two of them new this session: - `lumpedCalibration`'s `initial_values`-length mismatch guard (the S6466 IndexError fix) -- nothing called it with a mismatched list. - `read_lumped_model`'s `not isinstance(initial_condition, list)` guard (the S2589 always-true-condition fix) -- nothing called it with a non-list. - `_check_optimization_args`'s two dict-type checks -- pre-existing on this branch, never exercised by any test. Each is now pinned: the mismatch test confirms the optimiser is never reached and the error names both lengths; the non-list test parametrises over a tuple, an array and a string that all happen to hold five elements, so the length-first ordering the old bug depended on cannot silently come back; the dict-type tests cover both argument bundles and confirm the optimiser is unreached.
The mismatch-length test added in the previous commit built `_optimization_args()` inside the `pytest.raises` block alongside the call under test (S5778), and asserted both halves of the error message with `and` (S9073) -- the two patterns every other test in this file had already been fixed to avoid.
|
This was referenced Aug 30, 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
Every example run script opened with a block of hardcoded path and constant assignments, then
called the
read_*methods in the exact order the build-then-mutate pattern requires.Configuring a case study meant editing Python, and the ordering constraint got rediscovered per
script.
This branch moves that block into a YAML file and adds
Catchment.from_yaml:The layers are kept strictly apart, so no module reaches back into the one above it:
hapi.configdescribes data and nothing else — pydantic models plus their validationrules, importing nothing from
hapi. Being a leaf, it is imported normally at the top of themodules that use it.
Catchment.from_yamldoes the building — the construction, theread_*call order, theconceptual-model registry and the routing-label translation. It builds
cls, soCalibration.from_yamlreturns aCalibration.Runoverrides it to refuse, because itholds entry points called on a model built elsewhere.
MeteoInputs.from_configbuilds the drivers — the dispatch overmeteo.sourcelives withthe three loaders it chooses between, not in
Catchment.FlowNetworkis deliberately leftalone: its block maps to a single
from_rasterscall, so afrom_configthere would be analias with no dispatch to encapsulate.
Running the model stays the caller's job, exactly as in a hand-wired script. Which routing
function a lumped run uses is a run-time choice handed to
Run.runLumped, not an input, so itis still picked in the script.
Paths are resolved against the configuration file, not the process working directory, so a
configuration travels with the data it names and runs from anywhere. All four example scripts
load the YAML sitting beside them:
Validation
Using pydantic rather than plain dataclasses moves the rules that would otherwise be assertions
inside the builder into the schema, so a bad configuration is rejected before anything is read:
nmae)catchment.nmae: Extra inputs are not permittedspatial_resolution: semiInput should be 'lumped' or 'distributed'routing_method: kinematicInput should be 'muskingum' or 'maxbas'meteo.source: zarrInput should be 'rasters', 'netcdf' or 'netcdf_files'flow_networkgauges.tablesource: netcdf, nometeo.pathList should have at least 5 itemscatchment_area: -5Input should be greater than 0meteo.pathgauges.table/meteo.glob/ a grid driverwould be read by nothingsource: netcdf+glob,rasters+path, …routing_method: muskingum+parameters.maxbas: truemust agreemeteo.startaftercatchment.endends before it startsTwo rules are worth calling out because they are about silence rather than about a wrong value:
extra="forbid"alreadydoes that for a misspelled key; a correctly spelled but inapplicable one is the same mistake —
a line in the file with no effect. Each
meteo.sourcedeclares the fields its own branch offrom_configreads, andlumpeddoes the same formeteoandgauges. Only fields writtenexplicitly count, so a default is never held against an author.
routing_methodis derived fromparameters.maxbaswhen unstated. The two describe thesame choice from opposite sides, and the parameter-count check cannot catch a disagreement:
the counts differ by one and
maxbasselects which is expected, so a contradicting paircounts correctly and then reads the wrong parameter as the routing one.
Every input path is checked before the first reader runs, so two typos are reported together in
hundredths of a second rather than after the meteorological cube and the parameter folder have
been read.
Schema
Lumped and distributed runs disagree on the shape of two blocks, which is why the required
fields depend on
catchment.spatial_resolution:meteois a grid for distributed (raster folders or NetCDF, persource) and a single CSV ofcatchment-average drivers for lumped.
gaugesis a gauge table plus a folder of per-gauge files for distributed, and one dischargefile with no table for lumped.
All four Coello run scripts are ported, covering every variant: distributed Muskingum from one
combined NetCDF, distributed MAXBAS from raster folders (no flow-direction raster — triangular
routing sends every cell straight to the outlet), and the two lumped runs.
Dates may be quoted or not —
start: 2009-01-01reaches pydantic as adateand is writtenback out in the block's
fmt, because that is the spelling a YAML author writes first.assertis not validationsrc/hapicontained 53assertstatements doing input checking.assertis stripped underpython -O, so none of them were validation: an invalid input would have sailed past the guardand failed further in, on a shape error or a
None. All of them now raise —TypeErrorwhere atype is wrong,
ValueErrorfor a value or a length — and theRaises:entries advertisingAssertionErrorare corrected throughout.src/hapinow contains noassert.Repeated checks moved into helpers rather than becoming dozens of
ifblocks:_check_parameters_cover_gridand_check_lake_meteoinrun.py, and_check_optimization_argsincalibration.py.Three messages were wrong where they stood and are rewritten rather than carried over:
run_calibration/FW1Calibrationguardedtype(api_obj_args) is dictwith"store_history should be 0 or 1", and the solver arguments with"history_fname should be of type string".lumpedCalibrationcheckedbasic_inputsfor'Route'and'RoutingFn'while reporting"should contain ['p2','init_st','UB','LB']".maxbaschecks test>= 1but read"has to be larger than 1".One was a latent bug rather than only an
-Oproblem:read_discharge_gaugesguarded withassert hasattr(self, "GaugesTable"), but__init__sets that attribute toNone, so thecheck was always true and a caller who skipped
read_gauge_tablehit aTypeErroronNoneafew lines later. It now tests for
Noneand names the missing call.Defects found
All surfaced by running the code, not by reading it.
routing_methodis canonicalised to the exact literal the internals compare against, andanything outside
{muskingum, maxbas, kinematic}is now refused.Catchment.__init__storedit verbatim — unlike
spatial_resolution, which it lowercases — anddistrrm.SpatialRoutingtests
!= "Muskingum"case-sensitively. A lowercasemuskingumtherefore sent every celldown the MAXBAS branch and read
bankfull_depth, which isNoneoutside the flood model,raising
TypeError. This is the branch's breaking change — see Type of change below.read_discharge_gaugesfilledQGaugesby the wrong key. The frame was labelled from thegauge table's
columnbut filled withself.QGauges[int(name)], takingnamefrom theidcolumn. The two agree only when
columnis"id"; anything else produced the requestedcolumns entirely NaN plus a second, id-named set beside them, silently, and
extract_dischargethen computed metrics over the phantoms too.
Routing.calculate_weightsaccepted a MAXBAS below one. Below one whole step the trianglehas nothing to spread over:
0.5produced a single weight and an all-zero hydrograph withnothing raised,
0anIndexErrorabout axis 1, NaN a "cannot convert float NaN tointeger". The guard the three conceptual models and
triangular_routing_2already carried isnow at the one point
triangular_routing_1andDistMaxbas1resolve their weights through.Adding it immediately caught a real defect in the test suite: the lake tests built their
model from the Muskingum parameter set and then routed it triangularly, and
DistMaxbas1reads
parameters[..., -1]— MAXBAS in a MAXBAS set, the Muskingum X in that one. Every cellrouted with X = 0.2 and returned zeros, so four tests were asserting their shapes and flags
against output carrying nothing. They now build from the MAXBAS set (in-domain 1.4 to 2.4).
str | dt.datetimebut calledstrptimeunconditionally, sopassing the documented
datetimeraisedTypeError: strptime() argument 1 must be str.plot_hydrograph,read_discharge_gauges(thesplit=Truepath) andsave_resultsnow eachbranch on
isinstance(..., str).save_resultsconcatenated its output directory, sosome/dirwithout a trailingseparator wrote
some/dirResult_2009-01-01.tifbeside the directory rather than inside it.The names are now joined, the directory is created when missing, and a non-string
path—which
outputs.results_dircan be, being optional — is refused by name.read_lumped_modelmeasuredinitial_conditionbefore typing it, soNonereported"object of type 'NoneType' has no len()"; the type check that followed carried an
is not Nonethat could never be false.lumpedCalibrationindexedinitial_values[i]behind a non-empty guard only, so a listshorter than the bounds indexed out of range part-way through building the optimisation
problem, leaving it half-populated.
Dependencies: adds
pyyaml >=6.0andpydantic >=2.0,<3, neither previously declared. The capis at the major boundary because the schema is built on pydantic 2's API (
ConfigDictspreading,
protected_namespaces=(),model_fields_set,Field(min_length=…)on a list).Review
Two full
/review-roundspasses ran on this branch — 22 findings in round 1, 25 in round 2, allresolved — followed by a SonarCloud sweep that took the PR from 23 open issues to 6.
Six SonarCloud issues are left deliberately and are for the maintainer to accept or reject:
python:S1940×5 (routing.py:224,277,hbv.py:504,hbv_bergestrom92.py:441,hbv_lake.py:501) — Sonar asks formaxbas < 1instead ofnot maxbas >= 1. Both are falsefor NaN, and the negated form is deliberate so a NaN maxbas from a masked calibration cell
still fails naming the parameter. False positive.
python:S116(catchment.py:993) — renameQGaugesto snake_case. It is a public attributeof a published package, so renaming it is a breaking change out of this PR's scope.
Issues
hapi.config,Catchment.from_yaml,MeteoInputs.from_config)src/hapiand the defects it surfacedThis PR does not fix #211, the HBV off-by-one — it is unrelated and still reproducible: an
unrouted lumped run (
Run.runLumped(model)withRoute=0) fails withLength of values (1096) does not match length of index (1095).Type of change
The breaking change is the
routing_methodrejection, landed asrefactor(catchment)!: restrict routing_method to the three known methodswith aBREAKING CHANGE:footer so commitizen produces the right bump and changelog entry.CatchmentandCalibrationnow raiseValueErrorwhenrouting_methodis not one ofmuskingum/maxbas/kinematic(case-insensitive) instead of storing the string as given,and reading
model.routing_methodback returns the canonical spelling rather than what waspassed in. The canonicalisation is what makes
distrrm.SpatialRouting's exact comparisontrustworthy; the rejection is the part that is a contract change. The accepted set is documented
on the Catchment API page.
Everything else is additive.
hapi.configis new on this branch, so the interfaces that changedduring review never existed outside it. The
assertconversions change the exception typeraised on invalid input, which is why the affected tests are updated in the same commits.
How Has This Been Tested?
Each configuration was checked to assemble a model field for field identical to the
hardcoded block it replaces — parameters, drivers, gauges, flow network, initial conditions —
so the loader reproduces the same model rather than merely running.
export HAPI_DATA_DIR=src/hapi/parameters pixi run -e dev python examples/hydrological-model/coello/run/coello-distributed-model-run-netcdf.py pixi run -e dev main pixi run -e dev doctests pixi run -e dev mypy pixi run -e dev pre-commit run --all-files pixi run -e docs mkdocs build --strictdirectory — the one case config-relative paths make possible
readiness pass measured diff coverage on the changed lines and found three raise
branches with none (
lumpedCalibration's initial-values-length mismatch,read_lumped_model's non-list guard,_check_optimization_args's two dict checks);all three now have one
pythonandpython -Oimport hapi.runstrptimebugs, then confirmeddatetimeandstrnow workgauges.columncorruption and the all-zero MAXBAS hydrograph, thenconfirmed both fixed
hapi.configat 100 % line and branch coveragedoctestspixi task, a pre-commithook and the lint workflow, so they cannot drift again
mypyclean across all 23 source filespre-commit run --all-filesclean, including the local pytest hook andpixi.lockmkdocs build --strictexit 0Checklist:
The three unticked boxes are release steps: the version bump follows from the
BREAKING CHANGE:footer when
cz bumpruns, and the changelog is generated by commitizen rather than edited byhand. Documentation is updated — a new Run configuration page under Examples walks through a
configuration end to end, the two existing run pages link to it, and
docs/api/config.mdcarries the field-by-field reference.