ESCOMP / CTSM

Community Terrestrial Systems Model (includes the Community Land Model of CESM)
http://www.cesm.ucar.edu/models/cesm2.0/land/
Other
295 stars 298 forks source link

bug in crop fire modeling #2566

Open lifang0209 opened 1 month ago

lifang0209 commented 1 month ago

There is a bug in crop fires in CNFireLi2021Mod.F90 (the fire model developed for CLM5.1 and a possible default one in CESM3).

 baf_crop(c) = baf_crop(c) + cropfire_a1/secsphr * fhd * fgdp * patch%wtcol(p)
           if( fb * fhd * fgdp * patch%wtcol(p)  >  0._r8)then
              burndate(p)=kda
           end if
should be:
           if( fb * fhd * fgdp * patch%wtcol(p)  >  0._r8)then
 baf_crop(c) = baf_crop(c) + cropfire_a1/secsphr * fhd * fgdp * patch%wtcol(p)
              burndate(p)=kda
           end if

The bug causes baf_crop to be incorrectly accumulated, and the global constant cropfire_a1, calibrated using the inverse method, is significantly underestimated.

Currently, there are three options: (1) Submit a PR to report the bug. I just conducted the BGC historical run and will obtain the corrected cropfire_a1 and submit the PR tomorrow afternoon. (2) Submit a PR for the bug, along with developments in crop fire modeling and new crop fire timing inputs, sill in CNFireLi2021Mod (tomorrow afternoon). (3) Submit the final code of all fire model development (i.e., CNFireLi2024Mod) on Sunday. Which option do you prefer, and what are the rules for CLM regarding this?

cropfire_a1 was calibrated to match the observed and simulated global totals of crop fire burned area. Despite the incorrect additions of cropfire_a1/secsphr fhd fgdp patch%wtcol(p) when fb=0 ( fhd fgdp * patch%wtcol(p) always larger than 0 here), the overall impact on the total burned area was mitigated because cropfire_a1 was underestimated. The bug fix will correct these incorrect additions, ensuring that cropfire_a1 is more accurately represented.

wwieder commented 1 month ago

Thanks for reporting this, @lifang0209. Can you post here what the effect of fixing the bug will be (option 1 above). Specifically, this statement is confusing "As a result, the calculated global total of burned area remains correct. In other words, this error mainly affects the fire component." What is it that's different?

To clarify on your options above:

samsrabin commented 1 month ago

In general, it's better to submit many small PRs rather than one large PR. So if the fix for this can be easily split off from your main PR, that'd be preferable. Then if the new crop fire modeling and timing stuff can be split off into a different PR, that'd be great too. Thanks!

ekluzek commented 1 month ago

If I have this right "option 3" is really #2550 and "option 2" is parts of those changes from #2550.

Since, changing CNFireLi2021 would change answers in significant ways for clm5_0 (as well as clm6_0) I think we want to stick with just "option 1" there after we see how it affects results. The above seems like a small enough bug fix that it might be OK to change answers for clm5_0 as well as clm6_0.

samsrabin commented 1 month ago

Yes, in terms of how we time the merges that makes sense. But after the bugfix, it would be great to see a PR focused just on other crop fire improvements, separate from a PR with the other planned improvements.

lifang0209 commented 1 month ago

Thanks for reporting this, @lifang0209. Can you post here what the effect of fixing the bug will be (option 1 above). Specifically, this statement is confusing "As a result, the calculated global total of burned area remains correct. In other words, this error mainly affects the fire component." What is it that's different?

cropfire_a1 was calibrated to match the observed and simulated global totals of crop fire burned area. Despite the incorrect additions of cropfire_a1/secsphr fhd fgdp * patch%wtcol(p) when fb=0, the overall impact on the total burned area was mitigated because cropfire_a1 was underestimated. The bug fix will correct these incorrect additions, ensuring that cropfire_a1 is more accurately represented.

To clarify on your options above:

  • option 1 fixes the bug as you describe above
  • option 2 would fix the bug and adjust the results with some modification, but sill in CNFireLi2021Mod?
  • option 3 would introduce a new fire module (e.g. CNFireLi2024Mod)?

Thank you for your clarification request. It helped me articulate exactly what I wanted to express.

lifang0209 commented 1 month ago

The option 1 doesn't work. In the file CNFireLi2021Mod.F90, the crop fuel load (fuelc_crop) is calculated only when the crop model is inactive. When the crop model is active, fuelc_crop is set to 0 in the existing code. Consequently, fb, which depends on fuelc_crop, always equals 0. If we apply only the fix suggested by option 1, there will be no fires on managed croplands, which isn't desirable. On the other hand, if I do not fix the bug, baf_crop would be incorrectly accumulated, leading to crop fires occur when crops are growing, which is unreasonable. Therefore, I am now submitting a pull request for option 2.