search

NOAA-GFDL / GFDL_atmos_cubed_sphere

The GFDL atmos_cubed_sphere dynamical core code
Other
56 stars 118 forks source link

Use of uninitialized values in `fv_mapz_mod.cs_profile` when `iv == -3` #301

Closed spencerkclark closed 10 months ago

spencerkclark commented 11 months ago

Describe the bug

Debugging the source of non-reproducibility for a configuration of SHiELD turned up the use of uninitialized values in a local array in the cs_profile subroutine of the fv_mapz module. The issue occurs only when iv == -3, which corresponds to one of two possible modes for use when remapping the vertical velocity, the other being iv == -2. I have verified this with additional means, but the issue can be identified by inspecting the lines of code clipped below, considering the case when iv == -3: https://github.com/NOAA-GFDL/GFDL_atmos_cubed_sphere/blob/e94dc4b57c12cc399f6d319dc5733143fc6c718b/model/fv_mapz.F90#L1988-L2071 The issue specifically stems from the local gam array, which is declared in line 1988 without any initial assignment. When iv == -3 it gets values for level 1 in line 2025 and levels 2 through km - 1 (inclusive) in line 2034, but is not assigned values for level km prior to being used in line 2068. This leads to undefined / non-reproducible behavior at runtime.

To Reproduce

Run SHiELD in non-hydrostatic mode with fv_core_nml.kord_wz less than zero and abs(fv_core_nml.kord_wz) > 7, e.g. fv_core_nml.kord_wz = -9. This tells the model to use iv == -3 and cs_profile when remapping the vertical velocity:

https://github.com/NOAA-GFDL/GFDL_atmos_cubed_sphere/blob/e94dc4b57c12cc399f6d319dc5733143fc6c718b/model/fv_mapz.F90#L411-L424

Expected behavior

I would expect values to be defined for gam(:,km) when iv == -3 prior to it being used in line 2068.

System Environment

Describe the system environment, include:

lharris4 commented 11 months ago

@spencerkclark you are correct. gam(i,km) was incorrectly not specified in the iv .eq. -2 branch. I believe it should be specified the same way as in the interior (gam(i,k) = d4(i) / bet). This is an oversight on my part.

lharris4 commented 11 months ago

@laurenchilutti I haven't been assigned an issue before. Should I test the code and submit a patch? Thanks.

laurenchilutti commented 11 months ago

@lharris4 I did not look closely before assigning this - I had thought this was a PR and I was adding you as a reviewer. Please disregard the assignment.

lharris4 commented 11 months ago

@laurenchilutti Thank you.

spencerkclark commented 11 months ago

Thanks @lharris4 — I went ahead and made a PR with your suggested fix, and confirmed that it fixed the reproducibility issue: #302. I'm leaning on your expertise regarding the scientific correctness of this change.