Skip to content

r.sim: Fix null Manning's n and infiltration handling - #7981

Merged
wenzeslaus merged 4 commits into
OSGeo:mainfrom
wenzeslaus:simwe-mannings-null-mask
Oct 1, 2026
Merged

wenzeslaus merged 4 commits into
OSGeo:mainfrom
wenzeslaus:simwe-mannings-null-mask

Conversation

@wenzeslaus

@wenzeslaus wenzeslaus commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

Null cells in the Manning's n map were read as the internal no-data value -9999 and, unlike nulls in the other input maps, stayed in the simulation. Water there moved very slowly and uphill, the discharge came out negative, and the cells changed the time step and the reported mean Manning's n for the whole run. This masks them like the nulls of the other inputs, so they are excluded from the simulation, the outputs are null there, and walkers which reach them leave the simulated area.

The simulation already treated null cells in the infiltration map as no infiltration, but they made the reported mean infiltration negative. They are now read as zero infiltration, so only the reported mean changes. While this handling of infiltration is inconsistent with the other null handling, the "no infiltration" intuitively means zero as opposed unknown.

Runs without nulls in the Manning's n map give the same outputs as before.

Details (AI generated)
  • Change for Manning's n: one line in input_data() in raster/r.sim/simlib/input.c copies the nulls of the man map into the elevation grid with copy_matrix_undef_float_values(), as it is done for rain, dx, dy and the sediment inputs. The cells leave the domain, the outputs are null there and walkers reaching them leave the domain.
  • What was wrong: the nulls were read as -9999 and grad_check() inverted them to 1/n = -1e-4 (only 0 is checked). The velocity there was about 1e-5 of that for n = 0.1 and pointed uphill. It entered the mean velocity, hence the time step and the number of iterations, and the mean Manning's n in the run summary (r.sim: Add -p flag with format=json run summary and normalize history #7972) and the map history. The stored cchez * sqrt(sinsl) made the discharge output negative there.
  • Constant man_value: not affected, the grid is filled with the value, which is checked to be non-negative, so it has no nulls.
  • r.sim.sediment: reads its inputs through the same input_data(), so the fix applies to it as well.
  • Infiltration: nulls in the infil map were also read as -9999. The walker loop already skipped them, so they had no infiltration, but the value entered the reported mean infiltration, which came out negative (-499.9 instead of 2.6e-06 m/s in the new test). They are now set to zero when the map is read. Depth and discharge do not change.
  • Docs: one paragraph in NOTES of r.sim.water.md and r.sim.sediment.md lists the input maps whose null cells are excluded from the simulation (the outputs are null there and walkers that reach them leave the area), and for r.sim.water says that null infiltration means no infiltration, mirrored in the .html files.
  • Byte identity: with a Manning's n map without nulls (0.03 to 0.05 derived from the elevation), the nc_spm 10 m runs of r.sim.water (with and without dx and dy, nprocs=1) and r.sim.sediment give byte-identical depth, discharge, error, sediment flux, erosion/deposition, transport capacity and transport capacity limited erosion/deposition maps and an identical -p summary compared to main.
  • Measured on main before the fix: in the new test's synthetic plane, all 30 null-n cells had negative discharge, the time step was 0.116 s instead of 0.110 s, the reported mean Manning's n was 0.0526 instead of 0.05, and depth outside the patch changed by up to 0.0023 m. On the nc_spm elevation with null n over water, all null-n cells had negative discharge, the depth elsewhere changed by 0.88% in total and by up to 0.29 m per cell, and the time step was 0.30 s instead of 0.31 s.
  • Tests: the r.sim.water and r.sim.sediment pytest tests pass (58 in total with the new ones), as do both gunittest testsuites on nc_spm (r.sim.water 5 tests with 1 skipped, r.sim.sediment 1 test).

Tests

  • test_null_mannings_n_is_like_null_elevation (new, 1.2 s):
    • Why: no test used nulls in the Manning's n map, and a run with null elevation over the same cells gives the expected result to compare with.
    • Checks: on a 20x30 plane with a 6x5 patch in its middle, a run with null n in the patch gives exactly the same depth, discharge and JSON run summary (time step, iterations, mean velocity, mean Manning's n and so on) as a run with null elevation in the patch. It also checks that the outputs are null in the patch, the discharge is nowhere negative and the mean Manning's n is the value used elsewhere.
    • Setup: the partial derivatives are given as inputs. When they are computed from the elevation, null elevation is left out of the derivatives of the neighboring cells, while null n is not, so the two runs would differ at the edge of the patch (checked). One thread and a fixed seed make both runs deterministic.
    • Catches: null n cells left in the domain, anything that makes null n differ from null elevation in the walkers, time step or summary, and negative discharge. It fails on main at its first assertion.
    • Doesn't catch: r.sim.sediment is not run with null n, it goes through the same input code. The difference in the derivatives next to null n and null elevation is by design and not tested.
  • test_null_infiltration_is_no_infiltration (new, about 1 s):
    • Checks: a run with null infiltration in the patch gives exactly the same depth, discharge and JSON run summary, including the mean infiltration, as a run with zero infiltration there.
    • Catches: null infiltration treated differently from zero infiltration, in the walkers or in the reported mean. It fails on main at the summary comparison.

Squash-merge commit message

Title:

r.sim: Fix null Manning's n and infiltration handling

Body:

Nulls in the Manning's n raster map were read as the internal undefined
value -9999, but unlike nulls in the rainfall excess, partial derivative
and sediment input maps, they were not copied into the elevation grid,
so these cells stayed in the simulation domain with n = -9999. The
inverse 1/n was then -1e-4, which made the velocity there about 1e-5 of
the velocity for n = 0.1 and pointed it uphill. The cells also entered
the mean velocity, and so the time step and the number of iterations for
the whole domain, and the mean Manning's n in the -p summary and the map
history. The discharge output was negative in them.

Copy the nulls of the Manning's n map into the elevation grid as it is
done for the other input maps. The cells are now outside of the domain:
the outputs are null there and walkers which reach them leave the
domain. r.sim.water and r.sim.sediment share this code, so this applies
to both.

Nulls in the infiltration raster map were also read as -9999. The
walker loop skipped these cells, so they had no infiltration, but the
value entered the reported mean infiltration, which came out negative.
Set them to zero infiltration when the map is read.

Only runs with nulls in the Manning's n map change their outputs. With
a Manning's n map without nulls, the r.sim.water and r.sim.sediment outputs from the
nc_spm elevation are byte-identical to those before this change.

A new test checks that a run with a null patch in the Manning's n map
gives the same depth, discharge and run summary as a run with null
elevation over the same patch, with the partial derivatives given as
inputs so that they are the same in both runs. Another test checks that
null infiltration gives the same outputs and run summary as zero
infiltration.

Written with the help of Claude Code.

The analysis, code and tests were done with Claude Code (Opus) and reviewed by me.

Nulls in the Manning's n raster map were read as the internal undefined
value -9999, but unlike nulls in the rainfall excess, partial derivative
and sediment input maps, they were not copied into the elevation grid,
so these cells stayed in the simulation domain with n = -9999. The
inverse 1/n was then -1e-4, which made the velocity there about 1e-5 of
the velocity for n = 0.1 and pointed it uphill. The cells also entered
the mean velocity, and so the time step and the number of iterations for
the whole domain, and the mean Manning's n in the -p summary and the map
history. The discharge output was negative in them.

Copy the nulls of the Manning's n map into the elevation grid as it is
done for the other input maps. The cells are now outside of the domain:
the outputs are null there and walkers which reach them leave the
domain. r.sim.water and r.sim.sediment share this code, so this applies
to both.

Only runs with nulls in the Manning's n map change. With a Manning's n
map without nulls, the r.sim.water and r.sim.sediment outputs from the
nc_spm elevation are byte-identical to those before this change.

Written with the help of Claude Code.
A run with a null patch in the Manning's n map must give the same depth,
discharge and run summary as a run with null elevation over the same
patch. The partial derivatives are given as inputs, so that they are the
same in both runs. The test also checks that the outputs are null in the
patch, that the discharge is nowhere negative, and that the reported mean
Manning's n is the value used outside of the patch.

Written with the help of Claude Code.
Nulls in the infiltration raster map were read as the internal
undefined value -9999. The walker loop skipped these cells, so they
already had no infiltration, but the value entered the mean
infiltration reported in the -p summary and the map history, which
came out negative. Set the null cells to zero infiltration when the map
is read.

The depth and discharge do not change. A new test checks that null
infiltration gives the same outputs and run summary as zero
infiltration in the same cells.

Written with the help of Claude Code.
Replace the sentence about null Manning's n with one paragraph in NOTES
of each manual which lists the input maps whose null cells are excluded
from the simulation and says what happens there. For r.sim.water, it
also says that null infiltration means no infiltration.

Written with the help of Claude Code.
@github-actions github-actions Bot added raster Related to raster data processing Python Related code is in Python C Related code is in C HTML Related code is in HTML module docs markdown Related to markdown, markdown files tests Related to Test Suite labels Sep 30, 2026
@wenzeslaus
wenzeslaus enabled auto-merge (squash) October 1, 2026 03:10
@wenzeslaus
wenzeslaus merged commit ec90b1c into OSGeo:main Oct 1, 2026
25 of 27 checks passed
@wenzeslaus
wenzeslaus deleted the simwe-mannings-null-mask branch October 1, 2026 03:30
@github-actions github-actions Bot added this to the 8.6.0 milestone Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C Related code is in C docs HTML Related code is in HTML markdown Related to markdown, markdown files module Python Related code is in Python raster Related to raster data processing tests Related to Test Suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants