Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

frac_sno is always 0 for lake points #783

Closed
billsacks opened this issue Aug 7, 2019 · 1 comment · Fixed by #825
Closed

frac_sno is always 0 for lake points #783

billsacks opened this issue Aug 7, 2019 · 1 comment · Fixed by #825
Assignees
@billsacks
Labels
bug something is working incorrectly science Enhancement to or bug impacting science

Comments

@billsacks
Copy link
Member

Brief summary of bug

frac_sno appears to always be 0 for lake columns. This appears to cause incorrect results in various other lake variables.

General bug information

CTSM version you are using: ctsm1.0.dev055

Does this bug cause significantly incorrect results in the model's science? Yes

Configurations affected: All

Details of bug

I initially noticed this issue by examining LakeHydrology and seeing that frac_sno is never set there. I then confirmed that these three out-of-the-box initial conditions files have frac_sno identically 0 for all lake columns: clmi.B1850.0161-01-01.0.9x1.25_gx1v7_simyr1850_c190111.nc, clmi.I1850Clm50BgcCrop-ciso.1366-01-01.0.9x1.25_gx1v7_simyr1850_c190116.nc and clmi.I2000Clm50BgcCrop.2011-01-01.1.9x2.5_gx1v7_gl4_simyr2000_c180715.nc.

I then added this code (which isn't exactly what we want in the end) in order to see if these incorrect frac_sno values for lake have any effect on the simulation:

       if (snow_depth(c) > 0.0_r8) then
          frac_sno(c) = 1._r8
       else
          frac_sno(c) = 0._r8
       end if

This led to answer changes in many variables in a short test run. I have not yet investigated whether these differences are significant scientifically.

@swensosc thinks we should use the same snow cover fraction method for lakes as is used for non-lakes. I will plan to make that change. However, this will warrant a closer investigation to see the impact of this change.
@billsacks billsacks added bug something is working incorrectly tag: bug - impacts science labels Aug 7, 2019
@billsacks billsacks self-assigned this Aug 7, 2019
billsacks added a commit to billsacks/ctsm that referenced this issue Aug 15, 2019
@billsacks
Call SnowHydrology routine from LakeHydrology
054dc95 
This removes some duplicated code (and some other code that differed but
should be kept in sync between lake and non-lake moving forward). For
now this does more than it needs to do for lake, but this will let us
set frac_sno properly for lake without having lots of hard-to-maintain
code duplication.

To avoid changing answers too much, this resets frac_sno to 0 for lakes,
preserving the buggy behavior of ESCOMP#783.

However, this still changes answers by roundoff, due to changed order of
operations (now using newsnow rather than setting snow_depth from
dz_snowf, and setting dz_snowf differently).
billsacks added a commit that referenced this issue Aug 20, 2019
@billsacks
Merge branch 'frac_sno_ans_changes'
efca2bd 
Fix frac_sno bugs

Two bug fixes related to frac_sno, and a third change involving
replacing a frac_sno calculation with a simpler equation that is
algebraically equivalent:

(1) Fix lake frac_sno always being 0
    (#783). This fixes the albedo
    calculation (and possibly others) over snow-covered lake surfaces.

(2) Fix threshold for explicit snow pack initiation to use frac_sno_eff,
    not frac_sno (#785). For
    standard runs (which have use_subgrid_fluxes = .true.), this just
    changes answers for urban columns. This also changes answers more
    widely for runs with use_subgrid_fluxes = .false.

(3) Rewrite Swenson & Lawrence 2012 frac_sno equation to be more
    straightforward and less sensitive to roundoff errors
    (#784)

- Resolves #783 (frac_sno is always 0 for lake points)
- Resolves #785 (Threshold for explicit snow pack initiation
  should use frac_sno_eff, not frac_sno)
- Resolves #784 (Suggested algebraic rework of frac_sno
  calculation)
slevis-lmwg added a commit to slevis-lmwg/ctsm that referenced this issue Aug 21, 2019
@slevis-lmwg
Merge tag 'ctsm1.0.dev057' into soil_texture_interp_bug
be350df 
Fix frac_sno bugs

Two bug fixes related to frac_sno, and a third change involving
replacing a frac_sno calculation with a simpler equation that is
algebraically equivalent:

(1) Fix lake frac_sno always being 0
    (ESCOMP#783). This fixes the albedo
    calculation (and possibly others) over snow-covered lake surfaces.

(2) Fix threshold for explicit snow pack initiation to use frac_sno_eff,
    not frac_sno (ESCOMP#785). For
    standard runs (which have use_subgrid_fluxes = .true.), this just
    changes answers for urban columns. This also changes answers more
    widely for runs with use_subgrid_fluxes = .false.

(3) Rewrite Swenson & Lawrence 2012 frac_sno equation to be more
    straightforward and less sensitive to roundoff errors
    (ESCOMP#784)

Addressed conflicts in /doc
@billsacks
Copy link
Member Author

Although this was mostly fixed, I realized that there are still problems with frac_sno over lakes at the start of a run when using an old initial conditions file that had this problem: Since frac_sno is a restart variable, it starts as 0, then (based on looking at the code) it looks like it stays 0 until the next snowfall event.

This remaining issue should not have a significant impact on the science / climate, but it causes problems in some code rework I want to do, because it's problematic to have frac_sno = 0 when there is snow present. (Specifically: I want to change frac_sno_eff to be 0 when frac_sno = 0. But this causes divide by 0 in at least one place, where the assumption is that frac_sno_eff > 0 if there is a snow pack.)

@billsacks billsacks reopened this Oct 15, 2019
billsacks added a commit to billsacks/ctsm that referenced this issue Oct 17, 2019
@billsacks
For lakes, when reading restart file: set frac_sno=1 if snow_depth>0
ce91039 
Due to ESCOMP#783, old restart files can have frac_sno == 0 for
lake points despite having a snow pack. This can cause other problems,
so fix that here. (This mainly impacts restart files produced by
versions prior to the initial fix of ESCOMP#783 - i.e., prior to
ctsm1.0.dev057. However, in principle, restart files produced by
versions after that tag could still have this issue, since this
backwards compatibility code wasn't in place at that point, so frac_sno
could have persisted at 0 if there were no new snow since the last
restart file was written. So, to be safe, this backwards compatibility
code should be left in place until we can rely on all restart files
being from versions later than the tag where this backwards
compatibility was first implemented.)

The criteria snow_depth > 0 seems as good as any for determining if
there was actually snow present in a given column on the restart file.
billsacks added a commit to billsacks/ctsm that referenced this issue Oct 19, 2019
@billsacks
Base lake frac_sno reset on h2osno_total rather than snow_depth
337cdac 
ERI tests were failing the base-hybrid comparison when basing this reset
on snow_depth. My guess is that it's possible for snow_depth to be > 0
even when frac_sno is 0, at least for lakes. I'm hoping that using
h2osno_total will be more robust - i.e., that we should never have
frac_sno == 0 when h2osno_total > 0.

Addresses ESCOMP#783
billsacks added a commit to billsacks/ctsm that referenced this issue Oct 20, 2019
@billsacks
Base lake frac_sno reset on h2osno_total rather than snow_depth
b1f282c 
ERI tests were failing the base-hybrid comparison when basing this reset
on snow_depth. My guess is that it's possible for snow_depth to be > 0
even when frac_sno is 0, at least for lakes. I'm hoping that using
h2osno_total will be more robust - i.e., that we should never have
frac_sno == 0 when h2osno_total > 0.

Addresses ESCOMP#783
@billsacks billsacks mentioned this issue Oct 20, 2019
Merged
billsacks added a commit that referenced this issue Oct 24, 2019
@billsacks
Merge branch 'lake_frac_sno_finidat_fix'
837ad0d 
For lakes: when reading finidat, set frac_sno=1 if h2osno_total > 0

Due to #783, frac_sno used to be 0 for all lake points. That
was mostly fixed, but the issue is still present on initial conditions
files that were generated prior to that fix (which includes most or all
of our out-of-the-box restart files). This causes frac_sno to be 0 for
lake points at the start of the simulation, even if there is a snow pack
present. Currently this doesn't cause significant problems, but I'd like
to change the calculation of frac_sno_eff so that it is 0 if frac_sno is
0 - and then this frac_sno problem becomes an issue (causing divide by 0
in at least one place).

However, further problems were introduced if I tried to always apply
this correction: It appears that sometimes the restart file legitimately
has frac_sno == 0 for lake columns with non-zero snow cover. To avoid
changing answers for newer restart files, I am writing metadata to the
restart file documenting whether the fix has already been applied on
that restart file, and if so, we avoid reapplying the fix.

Getting this restart metadata correct was tricky, especially when
running init_interp. I have introduced a new module to encapsulate this
complexity. This can be used in general for writing / reading metadata
on which issues have been fixed on a given restart file.

Resolves #783
@samsrabin samsrabin added the science Enhancement to or bug impacting science label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@billsacks billsacks

Labels
bug something is working incorrectly science Enhancement to or bug impacting science
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Correct frac_sno = 0 for lakes on old initial conditions files billsacks/ctsm
2 participants
@billsacks @samsrabin