Skip to content

r.sim: disable broken nblock usage#7358

Open
petrasovaa wants to merge 3 commits into
OSGeo:mainfrom
petrasovaa:r.sim-nblocks
Open

r.sim: disable broken nblock usage#7358
petrasovaa wants to merge 3 commits into
OSGeo:mainfrom
petrasovaa:r.sim-nblocks

Conversation

@petrasovaa
Copy link
Copy Markdown
Contributor

TL;DR

Disabled the broken nblock >1 auto-split in r.sim.water/r.sim.sediment by forcing nblock=1. Kept the variables so that nblocks functionality can be enabled in follow up PRs.

Fixed the err formula to the original fortran's Monte Carlo stddev (fixes #4799). As a result error map is now 0 and will become more useful once nblocks will be enabled to be > 1.

Background

nblock originated in the Fortran ancestor as a memory workaround for running more walkers than the static buffer held; the C port inherited the structure but eventually mangled the per-block walker
accounting (rwalk was never actually divided), silently producing biased depth/discharge for users beyond the MAXW cap.

Summary of changes

  • Disable the broken auto-split path in r.sim.water / r.sim.sediment: force nblock = 1, drop the MAXW static cap, and remove the obsolete "ERROR: nwalk > maxw" doc reference.
  • Repair the err output to the Fortran's Monte Carlo standard-deviation formula (sqrt(|E[(gama·conn)²]/nblock − gama²|)); previously the C port had a wrong exponent and a missing
    post-loop step that produced a quantity with no clean meaning (fixes [Bug] r.sim.water: error output is not error but sum of depths? #4799). With nblock = 1 today it's zero everywhere; it becomes meaningful when nblock > 1 is reintroduced.
  • Introduce conn = nblock/iblock to gama in output_data for depth, discharge, conc, and flux outputs, matching the Fortran's intermediate-snapshot extrapolation. With nblock = 1 conn = 1.0
    and outputs are unchanged.
  • Preserve nblock, conn, and the iblock loop as scaffolding for sequential-replica reintroduction (Fortran-faithful, with working variance estimator) followed by thread-parallel replicas.

Behavior changes

  • nblock = 1 users (the only path that actually worked): depth, discharge, conc, flux, erdep unchanged. err output changes from pow(gama_final, 3/5) (which had no clean physical meaning)
    to all-zeros (correct stddev for one sample).
  • Users beyond the old MAXW cap: previously got silently biased outputs from the broken auto-split

Not in this PR

  • Reintroducing nblock > 1 (sequential-replica execution) — to follow.
  • Thread-parallel replica execution and the OpenMP race in the per-walker loop — separate follow-up.

@petrasovaa petrasovaa requested a review from wenzeslaus May 1, 2026 18:25
@github-actions github-actions Bot added raster Related to raster data processing C Related code is in C HTML Related code is in HTML module docs markdown Related to markdown, markdown files labels May 1, 2026
metzm
metzm previously approved these changes May 2, 2026
Copy link
Copy Markdown
Contributor

@metzm metzm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me, thanks!

Looking forward to a proposal enabling nblocks > 1.

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 raster Related to raster data processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] r.sim.water: error output is not error but sum of depths?

2 participants