Bug Fix - remove trunk loading from patch-level sum_fuel variable
#1180
Conversation
|
is sum_fuel used in other location(s) where it does want the trunks included? And if so, can you confirm that it is not used after the removal? |
It is not used anywhere else where we would want it to be included. It is actually sort of corrected for already here: So I suppose |
fire/SFMainMod.F90
Outdated
|
|
||
| ! remove trunks from patch%sum_fuel because they should not be included in fire equations | ||
| ! NOTE: ACF will update this soon to be more clean/bug-proof | ||
| currentPatch%sum_fuel = currentPatch%sum_fuel - litt_c%ag_cwd(4) |
There was a problem hiding this comment.
Would be safer to use tr_sf rather than 4, no?
There was a problem hiding this comment.
well....tr_sf is the trunks index in the SPITFIRE fuel array, not the litter array... though they are (implicitly) set up so that they are supposed to be the same order so I guess yes? I was going to fix this in the refactor
|
Okay so this is really baffling me, but I am not seeing really any DIFFs from my tests:
I have re-done the tests multiple times and get the same answers so just to test that I am not crazy I added this (i.e. - 5000) to the relevant line that I added:
I re-tried one of the passing tests and it did in fact fail this time:
But the weird thing is that it only affects a handful of variables: FATES_FUEL_AMOUNT, FATES_ROS, FATES_FIRE_INTENSITY: If these values are being updated so much I would think that other things would also be affected... I'm going to look at the output in xarray to see what is going on but if anyone else has any ideas please let me know... |
|
My one thought is that we don't have that many long tests so possibly we aren't running the model long enough to get a DIFF? |
|
But ultimately this is freaking me out that either there is another bug somewhere or that our tests do not actually cover enough |
|
Hmm. This change was only supposed to affect rate of spread, right? Not intensity / effects on vegetation? I guess if burned fraction is always either 0 or 1 both before and after the change, that would explain why only those metrics are affected. I don't expect that to be the case for a 6-month run though. Unless maybe it's from a cold start and there's just not that much woody fuel? |
|
Oh wait, I just saw your -50000 thing. That was smart! And now I'm also freaked out. Burned fraction wasn't affected?? |
|
So I'm just starting to look at the baseline right now, but in the 6 month baseline there is no FIRE_CLOSS so either yes we haven't run it long enough, or something weird is going on. If you want to take a look as well: baseline: -50000 test |
|
Incidentally I'm also now running a +50000 test... |
|
@samsrabin weird problems like this always deserve having someone else replicate the results. @adrifoster has already rerun several times, so if you (or someone else) could replicate the results that would be one good step to try. Also @adrifoster what is unique about this test from others? And can you show it with a smaller grid? Is this really the longest test at 30 days for FATES? |
Well this is a 6 month test... but yes I think (?) this is the longest FATES test we have. |
Okay...that one produced big diffs across all the variables I expected: Okay, so next thing... is this just a product of not running the model long enough (from bare ground - like all our tests) to get enough fuel to burn? Or is there an actual issue here. I suppose the thing to do would be to re-run an original test (without the crazy +/- 5000) for a longer time (20 years?) and see if this produces the expected DIFFs. I will do that (first I need to make a baseline for it). @samsrabin if you have time to replicate some of this just to make sure I am not missing something I am happy to collaborate, but otherwise we can discuss tomorrow at the SE meeting... |
Okay no I am wrong we do have a 25 month test... I guess even that isn't long enough. Will look at that to see. |
|
I think what you said about the baseline not having any So yes, re-running for 20 years would be a good idea. You could even do it at 10x15 rather than 4x5 if you want. Let's discuss at the SE meeting tomorrow, and if there's more weirdness we can discuss how I might be able to help. Some side notes:
|
|
Yes totally agree on the LUH2 test being the outlier and the key... I will look into that test. I also think we may not definitely need a longer test, but we could get around it with a test with initialized (cohorts and fuel I suppose) data. @ekluzek and I have been discussing this but I think 2 tests that would be great to have are:
|
|
Oh yeah, using pre-initialized data is a great idea! |
|
For CTSM issues that address this are: |
Also note #846 for the fates-side issue |
|
Okay so a 10-year test showed DIFFs we expect:
|
|
I don't think so, looks good, lets merge |
As laid out in #1178, trunks should (but currently aren't) removed from the patch-level variable
sum_fuelbefore being used to calculate rate of spread and fuel consumption.See Thonicke 2010 to confirm that this is how it is supposed to behave.
I added a quick fix at the end of the
charecteriscis_of_fuelsubroutine to do this.Description:
Just added one line to the end of the subroutine that removes the trunk litter. Currently this is not very nice looking, but since I am bringing in a big fire refactor I felt it was okay for now (??).
Collaborators:
@rgknox @samsrabin
Expectation of Answer Changes:
Yes, if fire is turned on at all, this should change rate of spread calculations, and thus fire intensity, fuel consumption, tree mortality, etc....
Checklist
Contributor
Integrator
Documentation
I don't believe documentation updates are needed since we are updating the code to reflect what we actually intend.
Test Results:
CTSM test hash-tag:
ctsm5.1.dev175CTSM baseline hash-tag:
ctsm5.1.dev175FATES baseline hash-tag:
sci.1.72.5_api.34.0.0Test Output:
Running tests now