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.

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

Suggested algebraic rework of frac_sno calculation #784

Closed
billsacks opened this issue Aug 9, 2019 · 1 comment
Closed

Suggested algebraic rework of frac_sno calculation #784

billsacks opened this issue Aug 9, 2019 · 1 comment
Assignees
Labels
code health improving internal code structure to make easier to maintain (sustainability)

Comments

@billsacks
Copy link
Member

The update of frac_sno to account for new snow looks like this:

https://github.com/ESCOMP/ctsm/blob/a87e67da23ab4c7e971351289dc051e2ebc5bf1c/src/biogeophys/SnowCoverFractionSwensonLawrence2012Mod.F90#L118-L120

I'm wondering if it would make sense to rewrite this as:

frac_sno(c) = frac_sno(c) + tanh(this%accum_factor * newsnow(c)) * (1._r8 - frac_sno(c))

I'm pretty sure that they do the same thing (though would love someone to double-check me on that), but I prefer the second form for two reasons:

  1. It is easier to understand what it's doing: It's starting with the existing frac_sno, then adding some fraction of area that is currently represented by (1 - frac_sno). In the current formulation, I got very confused by the two leading 1 - terms.

  2. The original form seems more sensitive to roundoff errors: If the tanh term is very small, note that the first expression results in frac_sno(c) = frac_sno(c), because 1 - tanh goes to 1 due to rounding error. The second expression is less sensitive to this roundoff-level issue. I particularly ran into this problem when trying a particular code refactoring (which I abandoned), when frac_sno was initially 0 and newsnow was very small: this should end up as frac_sno(c) = tanh(this%accum_factor * newsnow(c)), but due to rounding error, the original formulation gave exactly 0 in this case (causing a later divide by 0). (There may have been some other issue whereby newsno should have been truncated to 0 but wasn't.)

I wonder if we should change this... but I only want to do so if we're confident in the new form, because this will introduce answer changes and I want to be confident that this is truly just roundoff-level.

At the very least, I may want to add a comment to this effect.

However, before I either make the change or add a comment, I want feedback from @swensosc : Is there a reason why this is written the way it is? Do you have feelings on this proposed rewrite?

@billsacks billsacks added the code health improving internal code structure to make easier to maintain (sustainability) label Aug 9, 2019
@swensosc
Copy link
Contributor

It looks like an equivalent expression, so it would be okay to change. The argument for not changing is that the expression in the code is the result of the derivation in SL12 (eqn 3), which is the result of the probability based formulation. Maybe you could add a comment saying that it is equivalent to eqn 3 from SL12 when you change it.

billsacks added a commit that referenced this issue Aug 20, 2019
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health improving internal code structure to make easier to maintain (sustainability)
Projects
None yet
Development

No branches or pull requests

2 participants