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

Fire btran2 is only computed for exposed veg patches, but is used over all veg patches #1153

Closed
billsacks opened this issue Sep 18, 2020 · 7 comments
Assignees
Labels
bug something is working incorrectly science Enhancement to or bug impacting science

Comments

@billsacks
Copy link
Member

billsacks commented Sep 18, 2020

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:

do pi = 1,max_patch_per_col
do fc = 1,num_soilc
c = filter_soilc(fc)
g = col%gridcell(c)
if (pi <= col%npatches(c)) then
p = col%patchi(c) + pi - 1
! For non-crop -- natural vegetation and bare-soil
if( patch%itype(p) < nc3crop .and. cropf_col(c) < 1.0_r8 )then
if( .not. shr_infnan_isnan(btran2(p))) then
if (btran2(p) <= 1._r8 ) then
btran_col(c) = btran_col(c)+btran2(p)*patch%wtcol(p)
wtlf(c) = wtlf(c)+patch%wtcol(p)
end if
end if

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.

@billsacks billsacks added bug something is working incorrectly tag: bug - impacts science next this should get some attention in the next week or two. Normally each Thursday SE meeting. labels Sep 18, 2020
@ekluzek
Copy link
Collaborator

ekluzek commented Sep 18, 2020

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.

@billsacks
Copy link
Member Author

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.

@billsacks billsacks removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Sep 24, 2020
@billsacks billsacks self-assigned this Sep 24, 2020
@billsacks
Copy link
Member Author

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 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 - 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)?

@dlawrenncar
Copy link
Contributor

dlawrenncar commented Oct 1, 2020 via email

billsacks added a commit to billsacks/ctsm that referenced this issue Oct 2, 2020
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)
@billsacks
Copy link
Member Author

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.

@ekluzek
Copy link
Collaborator

ekluzek commented Oct 5, 2020

@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.

@billsacks
Copy link
Member Author

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.

billsacks added a commit that referenced this issue Oct 6, 2020
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)
@samsrabin samsrabin added the science Enhancement to or bug impacting science label Aug 8, 2024
samsrabin pushed a commit to samsrabin/CTSM that referenced this issue Sep 17, 2024
…histvar

Add a recruitment carbon flux history variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is working incorrectly science Enhancement to or bug impacting science
Projects
None yet
Development

No branches or pull requests

4 participants