dz used instead of dz_lake in LakeTemperatureMod.F90 in a loop with use_lch4 = .true.

[slevis-lmwg]

Brief summary of bug

I'm pretty sure that dz should have been dz_lake in this loop, though I'm not an expert on this section of the code.

General bug information

CTSM version you are using: ctsm1.0.dev049-4-gebe59970

Does this bug cause significantly incorrect results in the model's science? In use_lch4 = .true. cases.; if we were to turn on biogeochemistry with the NWP configuration, the bug would trigger the error shown below.

Configurations affected: use_lch4 = .true. with (for example) soil_layerstruct = '5SL_3m'

Details of bug

While performing testing in PR #759, the bug triggered this error:

Subscript 2 of the array DZ has value 10 which is greater than the upper bound of 9
65:Image PC Routine Line Source
65:cesm.exe 00000000019A2477 laketemperaturemo 960 LakeTemperatureMod.F90

when I ran this test
ERS_D_Ld10.f10_f10_musgs.I2000Clm50BgcCropGs.cheyenne_intel.clm-rm_indiv_lunits_and_collapse_to_dom
modified to have a soil layer profile of 9 (and later 5) soil layers.
So I believe that the bug triggers the error when the number of soil layers is less than the number of lake layers.

How I fixed the bug

I wish to know if reviewers agree with my fix:

Change this comment from
! The CH4 will diffuse directly from the top soil layer to the atmosphere, so
to
! The CH4 will diffuse directly from the top lake layer to the atmosphere, so

Change line 960 (in my version) from
lakeresist(c) = lakeresist(c) + dz(c,j)/kme(c,j) ! dz/eddy or molecular diffusivity
to
lakeresist(c) = lakeresist(c) + dz_lake(c,j)/kme(c,j) ! dz/eddy or molecular diffusivity

@billsacks recommended I ask @olyson and @dlawrenncar first.

[dlawrenncar]

I looked at the code and I agree with your fix. We do have to be careful because as of CLM4.5, lakes have soil/ground underneath them, using soil depths, not lake depths, but in this instance, the code should be calculating lake resistance so lake thickness is appropriate. I don't know that this is going to have much (any) impact, though, because I don't believe the lake CH4 emissions were ever fully implemented (i.e., there isn't any source carbon in the soil that could decompose and emit CH4, so lake CH4 should always be zero (though I could be wrong about that).

…

On Fri, Jul 12, 2019 at 11:53 AM Samuel Levis ***@***.***> wrote: Brief summary of bug I'm pretty sure that dz should have been dz_lake in this loop, though I'm not an expert on this section of the code. General bug information *CTSM version you are using:* ctsm1.0.dev049-4-gebe59970 *Does this bug cause significantly incorrect results in the model's science?* In use_lch4 = .true. cases.; if we were to turn on biogeochemistry with the NWP configuration, the bug would trigger the error shown below. *Configurations affected:* use_lch4 = .true. with (for example) soil_layerstruct = '5SL_3m' Details of bug The bug triggered this error: Subscript #2 <#2> of the array DZ has value 10 which is greater than the upper bound of 9 65:Image PC Routine Line Source 65:cesm.exe 00000000019A2477 laketemperaturemo 960 LakeTemperatureMod.F90 when I ran this test ERS_D_Ld10.f10_f10_musgs.I2000Clm50BgcCropGs.cheyenne_intel.clm-rm_indiv_lunits_and_collapse_to_dom modified to have a soil layer profile of 9 (and later 5) soil layers. So I believe that the bug triggers the error when the number of soil layers is less than the number of lake layers. How I fixed the bug I wish to know if reviewers agree with my fix: Change this comment from ! The CH4 will diffuse directly from the top soil layer to the atmosphere, so to ! The CH4 will diffuse directly from the top lake layer to the atmosphere, so Change line 960 (in my version) from lakeresist(c) = lakeresist(c) + dz(c,j)/kme(c,j) ! dz/eddy or molecular diffusivity to lakeresist(c) = lakeresist(c) + dz_lake(c,j)/kme(c,j) ! dz/eddy or molecular diffusivity @billsacks <https://github.com/billsacks> recommended I ask @olyson <https://github.com/olyson> and @dlawrenncar <https://github.com/dlawrenncar> first. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#760>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFABYVCBZAG2JZ3WEIKSBLLP7DAKPANCNFSM4ICOLJVQ> .

[slevis-lmwg]

Thank you @dlawrenncar

@billsacks do we fold the fix in PR #759 or should we handle separately?

[billsacks]

Thank you @dlawrenncar

@billsacks do we fold the fix in PR #759 or should we handle separately?

I'd say it depends on whether this changes answers for many tests, and if so, whether those answer changes are across-the-board or limited to one or a few diagnostic fields. If there are few or no answer changes in the test suite, then you can fold this into #759; otherwise, best to keep this separate.

[slevis-lmwg]

We have a mixed situation... On cheyenne:

• 76 tests have FAILed due to diffs from dev049 this far, and I have more than 50 still PENDing
• In 5 tests that I inspected:
  The clm2.h1 & cpl.hi files say, "the two files seem to be IDENTICAL"
  and the clm2.h0 file says, "A total number of 440 fields were compared
  of which 1 (WTGQ) had non-zero differences
  A total number of 2 fields could not be analyzed"

[slevis-lmwg]

My best guess from the fact that nothing else changes (and from @dlawrenncar's earlier comment):
WTGQ is a diagnostic field

[billsacks]

If it's just that one diagnostic field, I'd say it can be folded in with your other changes. I'm curious: what did you run the test suite on: was it just the dz_lake change, or was that combined with your other changes? If you went ahead and tested it separately anyway, then it seems worth making a tag with just that one change, but if it was combined with other things in your testing, then let's just combine it together in the end.

However: rather than relying on spot-checks of the cprnc.out files, I'd suggest using the summarize_cprnc_diffs tool that you can find in cime/tools/cprnc. Run it with the -h flag for help; briefly: you'll specify -basedir ... -testid ..., where the basedir argument should point to your /glade/scratch/slevis/tests_[whatever] directory, and the testid argument can contain shell wildcards to pick up all tests (if so, put it in single quotes, I think) (or you can run it separately with specific testids).

In this case, I would then look at the file that is sorted by variable name, and quickly look through there to confirm that the only diffs are in this one variable.

(FYI, this tool works by looking for certain important lines in all cprnc.out files in all test directories and then sorting these matching lines in various ways.)

[slevis-lmwg]

Yes, I created a separate branch and ran the test-suite on just the dz_lake change. I will try the summarize_cprnc_diffs tool next week. Thank you for suggesting that.

[slevis-lmwg]

Success: the summarize_cprnc_diffs tool shows only WTGQ with diffs in all the FAILed tests. I will open a PR and will run the Hobart test-suite next.