Fix uninitialized ilnb read on the thin-snow path in the RUC LSM - #2369
Open
FahrenheitResearch wants to merge 1 commit into
Open
Fix uninitialized ilnb read on the thin-snow path in the RUC LSM#2369FahrenheitResearch wants to merge 1 commit into
FahrenheitResearch wants to merge 1 commit into
Conversation
In SNOWTEMP and SNOWSEAICE, ilnb (the snow layer count) is assigned only inside if(snhei.ge.snth), but the tsnav recalculation at the end of each routine reads it whenever snhei > 0. On the thin-snow path (0 < snhei < snth) the read is undefined -- ilnb is intent(out) in SNOWTEMP and carries stale caller memory in SNOWSEAICE -- and when the leftover value is greater than 1 the two-layer formula runs with deltsn > snhei, an extrapolation across a negative layer thickness whose error grows without bound as the pack thins. Default ilnb to one layer before the branch in both routines. One layer is what the thin-snow block itself assumes (the snow 'is combined with the first soil layer'), and it reproduces exactly the tsnav that block already computes before the overwrite. Thick-path behavior is unchanged: both existing assignments still execute. Fixes wrf-model#2368
Collaborator
|
The regression test results: |
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.
Initialize the RUC LSM snow-layer count (
ilnb) so the thin-snow path no longer reads an undefined value into the TSNAV formula selection.TYPE: bug fix
KEYWORDS: RUC LSM, snow, uninitialized variable, ilnb, TSNAV, thin snow, restart reproducibility
SOURCE: FahrenheitResearch (independent)
DESCRIPTION OF CHANGES:
Problem:
As described in #2368: in
SNOWTEMPandSNOWSEAICE,ilnbis assigned only insideif(snhei.ge.snth), but the TSNAV recalculation at the end of each routine (:5716/:4410) reads it wheneversnhei > 0.. On the thin-snow path (0 < snhei < snth) nothing has assigned it —ilnbisintent(out)inSNOWTEMP(undefined on entry under the Fortran standard) and carries stale caller memory inSNOWSEAICE— and when the leftover value is greater than 1 the two-layer formula runs withdeltsn > snhei, an extrapolation across a negative layer thickness whose error grows without bound as the pack thins (14 K at 3 cm of snow, 60 K at 1 mm). It also makes the branch selection, and therefore TSNAV in the restart stream, dependent on compiler, optimization level, and the preceding column, so restarted runs need not reproduce continuous ones.On @weiwangncar's suggestion to set
ilnbinLSMRUCbefore the call toSFCTMP: I tried that form first, butilnbis a local variable ofSFCTMP(declared under!--- local variablesat:1385), not an argument passed in fromLSMRUC, so there is nothing inLSMRUCto set. And becauseSNOWTEMPdeclares itintent(out), an initialization upstream of the call chain would still be formally undefined on entry toSNOWTEMP— it would work with gfortran's by-reference scalar passing, but by the same accident that makes the current code usually work. Setting the default inside each routine that reads it is the smallest change that is well-defined on every path; happy to rework it into the caller-side form plus an intent change if the committee prefers.Solution:
Default
ilnb=1immediately before the layer-count branch in bothSNOWTEMPandSNOWSEAICE. One layer is what the thin-snow block itself assumes (its comment: the snow "is too thin to be treated separately, therefore it is combined with the first soil layer"), and it reproduces exactly the TSNAV that block already computes (:5191/:4103) before the overwrite. Thick-path behavior is unchanged: both existing assignments (ilnb=1/ilnb=2) still execute and take precedence.ISSUE: Fixes #2368
LIST OF MODIFIED FILES:
M phys/module_sf_ruclsm.F
TESTS CONDUCTED:
-O0) and drivesSFCTMPwith a 33-case, 160-column fixture covering land and sea ice, mosaic and non-mosaic, thick, thin, melting, and snow-free packs:-finit-integer=1vs-finit-integer=2: the same 45 TSNAV cells differ — the reproduction from the issue, confirmed onrelease-v4.8.0.-finit-integer=1/2, so the output no longer depends on what memory or the compiler left inilnb.ilnbreads 0 and already selects the one-layer formula): bit-identical across all 33 cases — no output changes on any thick-snow or snow-free path.RELEASE NOTE: Fixed an uninitialized read of the snow-layer count in the RUC LSM: on thin snow packs the pack-average snow temperature TSNAV could be computed with the two-layer formula, whose weights are degenerate there, producing errors that grow without bound as the pack thins and making restarts non-reproducible. The layer count now defaults to the single-layer form that the thin-snow path itself computes.