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.

Sign up for GitHub

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

Jump to bottom

bug in crop fire modeling #2566

Open
lifang0209 opened this issue May 30, 2024 · 6 comments · May be fixed by #2576
Open

bug in crop fire modeling #2566

lifang0209 opened this issue May 30, 2024 · 6 comments · May be fixed by #2576
Labels
tag: bug - impacts science bug causing incorrect science results type: bug something is working incorrectly

Comments

@lifang0209
Copy link

lifang0209 commented May 30, 2024 •
edited
Loading

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.
@ekluzek ekluzek added type: bug something is working incorrectly tag: bug - impacts science bug causing incorrect science results tag: next this issue should get some attention in the next week or two labels May 30, 2024
@wwieder
Copy link
Contributor

wwieder commented May 30, 2024

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:

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

@samsrabin
Copy link
Contributor

samsrabin commented May 30, 2024 •
edited
Loading

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
Copy link
Contributor

ekluzek commented May 30, 2024

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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.

lifang0209 added a commit to lifang0209/CTSM that referenced this issue Jun 3, 2024
@lifang0209
Fix bugs and develop crop fire modeling
0b84b5c 
In the CNFireLi2021Mod.F90, (1) fixed the bug described in ESCOMP#2566 to avoid incorrect accumulation of baf_crop and ensure that crop fires do not occur during the crop growing season; (2) confined crop fires to periods after harvest and before planting when crop module is active; (3) removed the dependency of baf_crop on fuel availability; (4) improved the modeling of the influence of socio-economic factors on crop burned area; (5) recalibrated the cropfire_a1 constant based on GFED5 crop burned area; (6) modify the declaration of CNFireArea in these F90 files to include the variable crop_inst, declare the variable crop_inst, and import and utilize crop_type from the module CropType.

In addition, the modules CNDriverMod.F90, CNFireLi2014Mod.F90, CNFireLi2016Mod.F90, CNFireNoFireMod.F90, FATESFireBase.F90, and FireMethodType.F90 include the subroutine CNFireArea. (6) is implemented in these modules.
@lifang0209 lifang0209 linked a pull request Jun 3, 2024 that will close this issue
Open
@samsrabin samsrabin removed the tag: next this issue should get some attention in the next week or two label Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
tag: bug - impacts science bug causing incorrect science results type: bug something is working incorrectly
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Fang summer '24 PR 1] Fix bugs and develop crop fire modeling lifang0209/CTSM
4 participants
@wwieder @samsrabin @ekluzek @lifang0209