Fix culvert discharge chatter: two drainage regressions since v2.2.0 - #360
Fix culvert discharge chatter: two drainage regressions since v2.2.0#360keesnederhoff wants to merge 1 commit into
Conversation
Newark culvert discharges chattered full-amplitude (drains 108, 15) from v2.3.0 onward. Two independent regressions, both restored to v2.0-v2.2 behaviour (output now matches v2.2.0): 1. sfincs_discharges.f90: v2.3.0 / PR #211 ("Moving gates") unified the drainage types and moved the available-volume clamp to AFTER the structure_relax relaxation. For volume-limited drains this applies the raw clamp output (empty->0, refill->full) every step. Clamp first, relax last, so the applied discharge is a smooth low-pass again. 2. sfincs_continuity.f90: restore the max(z_volume + qtsrc*dt, 0.0) floor on the source/drain volume update. The v2.3.0+ form let the cell volume overshoot negative and then gated the source off, making volume-limited drains ring (residual ripple). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Leynse
left a comment
There was a problem hiding this comment.
Nice fix Kees!
2 questions;
- Did you think about implications for mass conservation?
- And possible impact on other point sources as disfile etc, that are also affected by this?
Copilot review - One real concern:
1. Reordering relax after the clamp breaks the volume limit (and mass conservation)
sfincs_discharges.f90:626-645 — the clamp now applies to the target discharge, but the value actually written to qtsrc is a blend with the previous (possibly much larger) discharge. So the applied withdrawal can exceed the water available in nmin. The new max(..., 0.0) floor in continuity absorbs that on the donor side, but qtsrc(jout) = qq still delivers the full discharge to the receiving cell → net water creation whenever the drain runs a cell dry. Under the old order this couldn't happen.
Suggested alternative that fixes chatter and keeps the limiter authoritative: the chatter comes from relaxing toward the clamped previous discharge (qq0 = -qtsrc(jin) at :440/:540 feeds the clamped value back into the Bates update). Keep an unclamped qq_prev per structure, relax against that, then clamp last:
qq = w*qq_target + (1.0 - w)*qq_prev(idrn) ! smooth, unclamped
qq_prev(idrn) = qq
... volume clamp ... ! authoritative, applied last
qtsrc(jin) = -qq ; qtsrc(jout) = qq
2. Scope of the continuity change is wider than drainage structures
sfincs_continuity.f90:319 — the loop runs over nsrcdrn, i.e. all point sources, not just drains. Previously a positive source into a cell with z_volume < 0 was silently dropped; now it is applied. That is arguably the correct fix, but it's a behaviour change for plain src points that isn't mentioned in the PR title/description — worth calling out in the release notes. Also, momentum:697, infiltration:649 and meteo:1321 all explicitly test for negative z_volume, so silently flooring it at source cells hides mass errors that were previously visible.
Fix culvert discharge chatter: two drainage regressions since v2.2.0
Tim reported weird wiggles in the culvert discharges when running the refreshed testbed after the Galibier release. In the Newark drainage model, the drainage discharge on volume-limited culverts (drains 108 and 15) oscillates full-amplitude (0 to -16 m3/s) across the whole event, while the flood field (hmax) is essentially unaffected. Drains that are not volume-limited (e.g. 10, 100) stay smooth.
Both changes restore the v2.0-v2.2 behaviour:
max(z_volume + qtsrc*dt, 0.0)floor on the source/drain volume update.