Repository navigation
Fix column shift off-by-one error - #164
Open
BenjaminRodenberg wants to merge 5 commits into
Open
BenjaminRodenberg wants to merge 5 commits into
BenjaminRodenberg wants to merge 5 commits into
Conversation
The two vertical grid shifts in LOOP_SNOW have never been covered by a test. Add one that pins down both: snowfall_and_deposition must move every interior layer down one index and drop the deepest layer, and melt_sublimation must move every interior layer up one index and add a fresh layer at the base. Also assert the two budgets that follow from that: column mass changes by exactly the snowfall in minus the deepest layer out, and the water credited to runoff_irr_deep equals the water that really left the column. The test column deliberately gives every layer a distinct thickness, density and water content. This matters: the model's own initial state gives all deep layers the same values (INIT sets subD = C["Dice"] throughout and subZ[34:] = 0.8), and duplicating or dropping a layer that is identical to its neighbour is numerically invisible. A test built on a uniform deep column passes whether the shifts are right or wrong. subW is used as the tracer because nothing but the shift can touch it here: the column sits at the melting point so it has no cold content and no water can refreeze, no rain or melt adds water, and every layer's water stays below the mliqmax cap applied by compaction. That makes the permutation assertions exact rather than approximate. The upward shift is driven by sublimation rather than melt for the same reason - both feed the same shift_tot, but sublimation produces no liquid water. These tests fail on the current implementation, which leaves one interior index unwritten in each shift. The fix follows separately. Assisted-by: Claude Code:claude-opus-5.
Both blocks that shift the subsurface column by one index left exactly one interior index unwritten, so it silently kept its pre-shift value (indices zero-based). The downward shift in snowfall_and_deposition never assigned nl-1, dropping the layer at nl-2 and keeping the one at nl-1 that the runoff_irr_deep credit had already counted as gone. The upward shift in melt_sublimation never assigned nl-2, duplicating that layer and losing the one at nl-1. Widen both slices by one so every interior layer moves. The credit against subW_old[:, nl-1] is deliberately left alone: it was already right for the intended shift, and it was the shift that disagreed with it. layer_merging_and_splitting performs the same two shifts and was audited as correct. Per shift event the mass budget was off by subZ[nl-1] * (subD[nl-1] - subD[nl-2]), gaining mass on snowfall and losing it on melt, and the deep runoff credit by subW[nl-1] - subW[nl-2]. A 10-year single-column run at 500 mm w.e. per year drifted 8 % in column mass and was still diverging. The error vanishes when the two deepest layers are identical, which is how it survived undetected: the model's initial state makes them so. Both blocks are fixed together because their errors pushed thickness in opposite directions, so fixing one alone could expose a partly cancelling error. Assisted-by: Claude Code:claude-opus-5.
BenjaminRodenberg
force-pushed
the
fix-column-shift
branch
from
August 24, 2026 07:16
17c9f24 to
f905d5b
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Did not cause any problems; but I became aware of this potential bug while debugging #128.
@wardvp fix and test look plausible to me. Could you also take a look here?
Author's Todos