-
Notifications
You must be signed in to change notification settings - Fork 322
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
Fire btran2 is only computed for exposed veg patches, but is used over all veg patches #1153
Comments
This is the advantage of having the fire-specific setting of btran2 in the fire code, it makes this more obvious. When they are strewn around it's harder to tell if things like this match up. We should bring @lifang0209 into the conversation to ask about if this was intentional or not, and the impacts of changing it. |
From @dlawrenncar and myself: if there's no exposed veg, let's set btran2 to 1. I'll fold this into the upcoming btran2 tag. |
Actually, given @dlawrenncar 's point mentioned here #1170 (comment) - that btran2 is spval over bare patches, and so these bare patches are currently excluded from the average: I think the most consistent thing to do is to only include currently exposed veg patches in the btran2 column averages. This will treat veg patches that are either (a) leaf-free or (b) fully snow-covered the same as the bare ground patch, which feels like the right thing to do. One implication of this is that, if there is currently no exposed veg on a column, my proposal will lead to the block of code that forces @dlawrenncar - can you let me know if my plan of only averaging btran2 over exposed veg patches makes sense to you (as opposed to our earlier plan of prescribing btran2 = 1 over non-exposed patches)? |
Yes. Your proposal sounds correct to me.
…On Thu, Oct 1, 2020 at 2:00 PM Bill Sacks ***@***.***> wrote:
Actually, given @dlawrenncar <https://github.com/dlawrenncar> 's point
mentioned here #1170 (comment)
<#1170 (comment)> -
that btran2 is spval over bare patches, and so these bare patches are
currently excluded from the average: I think the most consistent thing to
do is to only include currently exposed veg patches in the btran2 column
averages. This will treat veg patches that are either (a) leaf-free or (b)
fully snow-covered the same as the bare ground patch, which feels like the
right thing to do.
One implication of this is that, if there is currently no exposed veg on a
column, my proposal will lead to the block of code that forces fire_m = 0
(because wtlf will be 0). Currently, in contrast, it looks like fire_m is
allowed to be non-zero even if there is currently no exposed veg, because
btran2 and wtlf are accumulated if a patch *ever* was exposed in the past.
@dlawrenncar <https://github.com/dlawrenncar> - can you let me know if my
plan of only averaging btran2 over exposed veg patches makes sense to you
(as opposed to our earlier plan of prescribing btran2 = 1 over non-exposed
patches)?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1153 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFABYVD4XARVFALVABS6VVLSITNVHANCNFSM4RRVLVTA>
.
|
This, or something like it, is important so that the fire code isn't trying to use btran2 from earlier time steps for patches that were in the exposed veg filter at one point but no longer are. The fix here is not the cleanest way to fix this issue, but represents a minimal set of changes to fix the issue. In a later commit, I'll try to fix this in a better way (which should be bit-for-bit with this fix). Resolves ESCOMP#1153 (Fire btran2 is only computed for exposed veg patches, but is used over all veg patches)
Even though btran2 is now only referenced over exposed veg patches, I still need to set it to some value over non-exposed patches for the sake of history diagnostics. The original btran variable is set to 0 over these non-exposed patches, so that's what I'll do here, too. |
@billsacks I thought it set the entire array of btran2 to spval, just before setting up the history variable. So shouldn't that be what's done? Or are you talking about patches that go from non-exposed to exposed and vice-versa? Those would've just retained their previous value until they are exposed again and can change, which isn't likely the right thing. |
Yes, that's exactly what I mean. |
CNFire: btran2 fixes and general cleanup (1) Call routine to calculate fire's btran2 from CNFireArea; this has a few advantages: - It makes the logic of CNFireArea more clear (rather than depending on a btran2 variable that is calculated from some other part of the code) - This avoids having the biogeophysics depend on the biogeochemistry - This lets us avoid doing this btran calc if using no-fire – or other, future, fire methods that don't need it Note regarding testing: In the initial step, I kept this calculation dependent on a saved version of h2osoi_vol to avoid changing answers; I changed this in the answer changes in step (2), as noted below. (2) Answer-changing fixes to CNFire's btran2 calculation and use: (a) Calculate fire btran2 using updated h2osoi_vol (this is an answer-changing cleanup step from (1)) (b) TEMPORARY CHANGE (reverted in the cleanup in (3)): Reinitialize fire btran2 to spval for all patches in each time step, so that the fire code isn't trying to use btran2 from earlier time steps for patches that were in the exposed veg filter at one point but no longer are. One implication of this is that, if there is currently no exposed veg on a column, the new code leads to the block of code that forces fire_m = 0 (because wtlf will be 0). Previously, in contrast, it looks like fire_m was allowed to be non-zero even if there is currently no exposed veg, because btran2 and wtlf were accumulated if a patch ever was exposed in the past. (c) Limit fire btran2 to be <= 1, rather than letting it be slightly greater than 1. (Due to a conditional in CNFireArea, these slightly-greater-tan-1 values were being ignored when computing btran_col, rather than averaging in a 1 value.) (3) Non-answer-changing fire code cleanup: (a) Cleanup of the btran2 fixes, including reverting the TEMPORARY CHANGE noted in (2b), instead relying on a better mechanism: just doing the calculations of btran_col and wtlf over the exposedvegp filter. Also, get rid of the checks for shr_infnan_isnan(btran2(p)) and btran2(p) <= 1 (allowed by the other changes in (2) and (3)). (b) Set btran2 to 0 over non-exposed-veg points: this changes answers for the BTRAN2 diagnostic field, but nothing else. (This follows what is done for the standard BTRAN.) (c) Move calc_fire_root_wetness for CNFireLi2021 into the base type to avoid future bugs (assuming that the next fire module will extend the base class but will also want to use this new version of calc_fire_root_wetness). (d) Change fire looping structure to be more standard (4) Remove some very expensive tests from aux_clm, putting some in the new ctsm_sci test list instead (5) A bit of other minor cleanup Issues fixed: - Resolves #1139 (Decrease expense of ne0ARCTICGRISne30x8 test and C96 tests) - Resolves #1153 (Fire btran2 is only computed for exposed veg patches, but is used over all veg patches) - Resolves #1170 (CNFire code: btran2 should not be skipped when it's greater than 1) - Partially addresses #1142 (Add ctsm_sci test list that would be used for releases and making sure important resolutions run well)
…histvar Add a recruitment carbon flux history variable
Brief summary of bug
From examining the fire code, it looks like btran2 is used over all vegetated patches, but it is only computed each time step over the exposed veg patches. I think this means that, for non-exposed patches, it is using the btran2 value from whenever the patch was last exposed. This seems scientifically questionable.
General bug information
CTSM version you are using: master
Does this bug cause significantly incorrect results in the model's science? Yes (I think... but impact is probably relatively small)
Configurations affected: All configurations with fire active
Details of bug
btran2 is accessed for all vegetated patches:
CTSM/src/biogeochem/CNFireLi2016Mod.F90
Lines 346 to 360 in 5cd28a0
yet I'm pretty sure its calculation is just done over the exposed veg filter (in calc_root_moist_stress_clm45default).
#1151 refactors this, but I don't think it changes the points over which btran2 is calculated. In fact, I noticed this when reviewing #1151, because that PR makes the filter over which btran2 is calculated more explicit.
I think this means that the btran2 values over non-exposed vegetated patches are taken from some previous time step – whenever the given patch was last exposed. I'm not sure how much of an impact this has on the results, but it feels wrong.
Probably what should be done is that some assumed btran2 value is used over non-exposed vegetated patches – probably 1. In principle this could require retuning of the fire model, but I hope the impact would be relatively small because there hopefully aren't many fires in these non-exposed (which I think implies snow-covered) points.
I'm hopeful that this change would also allow us to remove btran2 from the restart file.
The text was updated successfully, but these errors were encountered: