You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
fl_valley_confine(field = "channel_width") defaults to a column that is wrong for the flood
model, and produces a smaller floodplain silently.
field names the column rasterized into stream_r. That raster is then handed to fl_flood_model() -> fl_flood_surface(), where it is used as the drainage-area term in the
Hall bankfull regression (R/fl_flood_surface.R:79):
contrib<-terra::ifel(streams<0, 0, streams) # <- the rasterized `field`bankfull_width<- (contrib^0.280) *0.196*pcp
So field must be upstream area. The default supplies channel width instead — a valid number that
means something else entirely, raised to the 0.280 power without complaint.
Measured on the bundled tile
Re-measured on current code (0.5.0, after the #49 units fix). The figures this issue was filed with
were taken before that fix and no longer describe the package; the defect is the same, its
consequence is smaller.
At package defaults, flood_factor = 6:
field / area_field
precip
cells
ha
upstream_area_ha
map_upstream
28,727
287.3
channel_width (the default)
map_upstream
17,206
172.1
upstream_area_ha
none
19,838
198.4
channel_width (the default)
none
14,789
147.9
The default returns 59.9% of the floodplain with precipitation supplied, with no error and no
warning. Every wrong-column run is a strict subset — 0 cells gained anywhere.
As filed, at flood_factor = 4 on 0.4.1's over-mapping code, it was 222.1 ha against 476.8 ha (47%).
Column ranges in inst/testdata/streams.gpkg show why nothing catches it — both are plain positive
numerics:
column
range
upstream_area_ha
1,928.8 .. 110,337.4
channel_width
4.1 .. 31.3
Why it has survived
Every caller passes field explicitly: all @examples, both vignettes, and the driver at floodplains/scripts/floodplain_lcc/02_floodplain_model.R:134 all use field = "upstream_area_ha". The default is never exercised, so nothing has been produced
incorrectly — but it is live for the first person who omits the argument, and the roxygen gives them
no reason to think they should not.
Current documentation for the parameter:
@param field Character. Column name for [fl_stream_rasterize()] when streamsissf. Default "channel_width"`.
It never says the flood model reinterprets this column as drainage area.
A plausible-looking number flowing into a formula that expects a different quantity: no error, no
warning, output that looks entirely reasonable. In #41 it was a zero-friction cell read as a stream
seed; here it is a channel width read as a drainage area. Both are only visible if you already know
to look.
Decision — shipped in 0.6.0 as (1) + (3), with one correction
Three options were weighed at filing: (1) make it required, (2) keep the default behind a magnitude
guard, (3) rename it so the required quantity is stated at the call site. (2) was rejected — a
genuinely small headwater basin is ~10 ha, overlapping the channel-width range, so the guard cannot
separate its two inputs and would fail exactly on the small streams where NA handling is already
awkward.
What shipped is (1) + (3): a required area_field, with the roxygen stating hectares and naming fl_flood_surface() as the consumer, and @seealso in both directions between fl_valley_confine() and fl_stream_rasterize(). fl_stream_rasterize() keeps its "channel_width" default — it is deliberately generic, and the hazard was always the composition.
Two corrections to the reasoning above, both found during the work:
The audit behind "breaking for ... nobody in our code" was about the unused default. The
rename also breaks every working explicit caller — all vignettes and examples, and the driver at floodplains/scripts/floodplain_lcc/02_floodplain_model.R:148. So field was kept for one release
as an alias that warns and forwards, reproducing the same delineation exactly. Removal is tracked
in Remove the deprecated field argument from fl_valley_confine() #53.
The options list had no category for the residual: fl_valley_confine() also accepts a
pre-rasterized SpatRaster, which carries the identical defect one call earlier and cannot be
checked by any argument change. That branch cannot inspect values, but it can inspect the layer's name, since fl_stream_rasterize() names its output after the column it burned — so a layer
named channel_width now warns. This is not option (2) in disguise: it is exact where a magnitude
test cannot be, and it makes no claim about a raster burned from any other wrong column, which
remains undetectable and is documented as such.
Resolved while filing: the units question
The original filing flagged that inst/research/vca_parameter_rationale.md annotates the Hall
regression as taking km2 and cm/yr while fl_flood_surface.R fed it hectares and mm. That was the
separate defect #49, fixed in 0.5.0 — fl_flood_surface() now converts internally and callers still
pass upstream_area_ha and precipitation in mm. Nothing outstanding there.
Problem
fl_valley_confine(field = "channel_width")defaults to a column that is wrong for the floodmodel, and produces a smaller floodplain silently.
fieldnames the column rasterized intostream_r. That raster is then handed tofl_flood_model()->fl_flood_surface(), where it is used as the drainage-area term in theHall bankfull regression (
R/fl_flood_surface.R:79):So
fieldmust be upstream area. The default supplies channel width instead — a valid number thatmeans something else entirely, raised to the 0.280 power without complaint.
Measured on the bundled tile
Re-measured on current code (0.5.0, after the #49 units fix). The figures this issue was filed with
were taken before that fix and no longer describe the package; the defect is the same, its
consequence is smaller.
At package defaults,
flood_factor = 6:field/area_fieldupstream_area_hamap_upstreamchannel_width(the default)map_upstreamupstream_area_hachannel_width(the default)The default returns 59.9% of the floodplain with precipitation supplied, with no error and no
warning. Every wrong-column run is a strict subset — 0 cells gained anywhere.
As filed, at
flood_factor = 4on 0.4.1's over-mapping code, it was 222.1 ha against 476.8 ha (47%).Column ranges in
inst/testdata/streams.gpkgshow why nothing catches it — both are plain positivenumerics:
upstream_area_hachannel_widthWhy it has survived
Every caller passes
fieldexplicitly: all@examples, both vignettes, and the driver atfloodplains/scripts/floodplain_lcc/02_floodplain_model.R:134all usefield = "upstream_area_ha". The default is never exercised, so nothing has been producedincorrectly — but it is live for the first person who omits the argument, and the roxygen gives them
no reason to think they should not.
Current documentation for the parameter:
It never says the flood model reinterprets this column as drainage area.
Same class as #41
A plausible-looking number flowing into a formula that expects a different quantity: no error, no
warning, output that looks entirely reasonable. In #41 it was a zero-friction cell read as a stream
seed; here it is a channel width read as a drainage area. Both are only visible if you already know
to look.
Decision — shipped in 0.6.0 as (1) + (3), with one correction
Three options were weighed at filing: (1) make it required, (2) keep the default behind a magnitude
guard, (3) rename it so the required quantity is stated at the call site. (2) was rejected — a
genuinely small headwater basin is ~10 ha, overlapping the channel-width range, so the guard cannot
separate its two inputs and would fail exactly on the small streams where NA handling is already
awkward.
What shipped is (1) + (3): a required
area_field, with the roxygen stating hectares and namingfl_flood_surface()as the consumer, and@seealsoin both directions betweenfl_valley_confine()andfl_stream_rasterize().fl_stream_rasterize()keeps its"channel_width"default — it is deliberately generic, and the hazard was always the composition.Two corrections to the reasoning above, both found during the work:
rename also breaks every working explicit caller — all vignettes and examples, and the driver at
floodplains/scripts/floodplain_lcc/02_floodplain_model.R:148. Sofieldwas kept for one releaseas an alias that warns and forwards, reproducing the same delineation exactly. Removal is tracked
in Remove the deprecated
fieldargument from fl_valley_confine() #53.fl_valley_confine()also accepts apre-rasterized
SpatRaster, which carries the identical defect one call earlier and cannot bechecked by any argument change. That branch cannot inspect values, but it can inspect the layer's
name, since
fl_stream_rasterize()names its output after the column it burned — so a layernamed
channel_widthnow warns. This is not option (2) in disguise: it is exact where a magnitudetest cannot be, and it makes no claim about a raster burned from any other wrong column, which
remains undetectable and is documented as such.
Resolved while filing: the units question
The original filing flagged that
inst/research/vca_parameter_rationale.mdannotates the Hallregression as taking km2 and cm/yr while
fl_flood_surface.Rfed it hectares and mm. That was theseparate defect #49, fixed in 0.5.0 —
fl_flood_surface()now converts internally and callers stillpass
upstream_area_haand precipitation in mm. Nothing outstanding there.