-
Notifications
You must be signed in to change notification settings - Fork 9
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
negative snow/water test, beta #497
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you, @kmdeck ! I only had a couple of minor comments.
snow_cover_fraction = heaviside(σS) | ||
return (1 - snow_cover_fraction) * β(W, W_f) + snow_cover_fraction * 1 | ||
return (1 - snow_cover_fraction) * β(W, FT(0.75 * W_f)) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to make 0.75 and 0.1 tunable parameters? It would be good to comment on these choices in the doc string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make these tunable params - and p
also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ill add them to the parameter struct
@@ -526,7 +526,7 @@ function next_albedo( | |||
) where {FT} | |||
(; α_snow, α_bareground) = model_albedo | |||
(; σS_c) = parameters | |||
σS = Y.bucket.σS | |||
σS = max.(Y.bucket.σS, FT(0))# if σS is small and negative, clip to zero for albedo estimation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be eps(FT), so it's consistent with the heaviside function? (also below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. I think both are consistent in a way:
In the heaviside, we are saying that snow < eps(FT) means the snow cover fraction is treated as zero. Here, we are saying that snow being negative means treat it like zero snow. A negative snow would also give zero snow cover fraction, so those are consistent.
If we treat negative snow as eps(FT) as the snow content, the heaviside will return 0 still, since we compare eps(FT) < eps(FT), which is false. so that seems fine too.
Im not sure! if we want zero snow it almost seems like we should return zero snow. using eps(FT) as the heaviside threshold is in case the snow damps to zero but does not exactly reach == 0.
I
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant more for the case of 0 < σS < eps(FT)
, whether it should be treated as 0 or nonzero. It also appears in partition_surface_fluxes()
. That said, it looks like the snow_cover_fraction
which is only used to scale evaporation, so I think that's ok, as long as the user doesn't get confused how to treat small values of snow. Maybe a comment would do. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in that case, should the comment on Line 457 (# Equation (3) of the text, snow melt already multipied by snow_cover_fraction
) say max(0, σS)
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I think I understand. You're right that we are being inconsistent in the 0 < σS < eps(FT)
regime. In principle, the snow_cover_fraction function can be anything (e.g. sigmoid function), and clipping negative values of σS to zero should still be a valid thing to do (they shouldnt depend on each other). I think they do because some of parameterizations depend on σS (e.g. the albedo, the beta function) and some depend on snow cover fraction. partition_surface_fluxes
uses both in the same function.
Here is what I did to try and fix this:
partition_surface_fluxes
now works in units of emitting/absorbing area (snow). This is consistent with how we handle fluxes in the documentation and everywhere else - they are fluxes per emitting area and we multiply by emitting area fraction (1-σ) or σ to get fluxes per unit ground area when used in setting tendencies. σ is now applied outside the partition_surface_fluxes now, in Bucket.jl lines 457, 468, 474, and 437.- The albedo model (linear interpolation between snow and bareground albedo) fundamentally conflicts with a heaviside function for snow cover fraction, so I left this alone for now. The inconsistency remains but it is O(eps) and doesnt affect any conservation. I left a note in the documentation.
- The other inconsistency arises in the beta function, but this function is independent of the snow cover fraction and implemented for numerical reasons only, so I dont know that they need to be consistent. if 0 < σS < eps(FT), this function will be called for snow but it wont be used because it is multiplied by the SCF.
If we want to use the bucket model as a stepping stone for the full land model for testing different fractional coverages within a grid cell, we can make this consistent and use the SCF for the albedo function, at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable to me. Thank you for digging into this, Kat!
2461693
to
08eaae6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @kmdeck , thank you! I only had one more clarification comment.
5497f9f
to
6fab7d5
Compare
Purpose
May close CliMA/ClimaCoupler.jl#472
The default parameters of bucket model make it so the beta function recovers the SLIM model (linear beta, switch occurs at 0.75*bucket capacity, and no snow beta).
To-do
TBD: make beta_snow and beta_bucket different functions
TBD: make them functions the user can pass in (for now, the user can control this with parameters of the function)
Content
W = max(W, 0)
in functions where being negative would return an incorrect answer and we want to treat the negative number like zeroThis is what we had before for water, with
x_c = 0.75*W_f
,x = W
, andp=1
. We've now set p=2 to damp nonlinearly to zero. The branch is used for the bucket because the land evaporates at the potential rate if the bucket is full enough (beta=1), but then drops as it gets drier. The transition happens at x_c [SLIM supplement text Equation 36].For snow, the sublimation rate should be the potential rate (beta = 1) until the snow vanishes. To achieve this, we use the same function for beta, but use
x_c = 0.1 S_c
, whereS_c
is the parameter used to control the albedo of the land surface. For snow levels about S_c, the albedo is the snow albedo, as S drops far below S_c, it becomes the bareground albedo. My only concern here is that the physical situation could be S<<S_c (patchy, thin snow), and the albedo-> bareground albedo, but the vapor flux -> 0, because we treat the snow cover fraction as zero or 1 in all cases but the albedo. In reality, in this situation, the vapor flux should tend towards the bareground vapor flux.Review checklist
I have:
In the Content, I have included