Skip to content

fix(hapi): replace assert-based input validation with raises #216

Description

@MAfarrag

Context

src/hapi used assert for input validation in 53 places. assert is stripped under
python -O, so none of those were doing real validation -- an invalid input sailed past the
guard and failed further in, on a shape mismatch or a None. PR #214 converted every one. This
issue documents that change and the defects it surfaced along the way.

What Changed

  • Every assert in src/hapi doing input checking now raises TypeError (wrong type) or
    ValueError (wrong value/length) instead. src/hapi now contains no assert.
  • Repeated checks were pulled into helpers instead of becoming dozens of near-identical if
    blocks: _check_parameters_cover_grid and _check_lake_meteo in run.py,
    _check_optimization_args in calibration.py.
  • Three messages that were already wrong, independent of the -O issue, are corrected rather
    than carried over verbatim (the store_history/history_fname messages in
    run_calibration/FW1Calibration, lumpedCalibration's basic_inputs message, and the
    four maxbas checks' wording).

Defects found while doing this

  • Catchment.__init__ now canonicalises and rejects routing_method. It used to store
    the string verbatim, so a lower-case "muskingum" silently routed every cell down the
    MAXBAS branch (distrrm.SpatialRouting compares != "Muskingum" case-sensitively) and
    raised TypeError on bankfull_depth, which is None outside the flood model. Breaking
    change
    : landed as refactor(catchment)! with a BREAKING CHANGE: footer.
  • read_discharge_gauges filled QGauges by the wrong key -- labelled from
    gauges.column but filled by int(id), so any column != "id" produced the requested
    columns entirely NaN plus a second, id-named set beside them, silently.
  • Routing.calculate_weights accepted a MAXBAS below one -- 0.5 produced an all-zero
    hydrograph with nothing raised. Fixing it exposed a real defect in the lake test fixtures,
    which had been routing on the wrong parameter set and asserting against empty output.
  • save_results concatenated its output directory instead of joining it, so some/dir
    without a trailing separator wrote some/dirResult_2009-01-01.tif beside the directory
    rather than inside it.
  • Three methods advertised str | dt.datetime but called strptime unconditionally
    (plot_hydrograph, read_discharge_gauges's split=True path, save_results); each now
    branches on isinstance(..., str).

Affected locations

File Notes
src/hapi/catchment.py routing_method canonicalisation, QGauges fix, save_results fix, datetime branches
src/hapi/routing.py calculate_weights guard
src/hapi/calibration.py _check_optimization_args, two Sonar-flagged guards
src/hapi/run.py _check_parameters_cover_grid, _check_lake_meteo
src/hapi/inputs.py, src/hapi/rrm/*.py remaining assert -> raise conversions

Verification

  • Every converted check verified to raise under both python and python -O
  • Reproduced the gauges.column corruption, the all-zero MAXBAS hydrograph and both
    strptime bugs, then confirmed each fixed
  • Full suite green (554 tests) after the conversion
  • Two full /review-rounds passes and a SonarCloud sweep (23 open issues down to 6)

Implemented in #214.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingrefactor

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions