Refactor SPITFIRE - overhaul of fire behavior sections (but bit4bit) #1326
Conversation
|
|
||
| if (windspeed_km_hr < 1.0_r8) then | ||
| LengthToBreadth = 1.0_r8 | ||
| else |
There was a problem hiding this comment.
Leading whitespace is misaligned in this block.
| if (i /= fuel_classes%trunks()) then | ||
| ! don't include 1000-hr fuels | ||
| ! convert loading from kgC/m2 to g/cm2 | ||
| tau_l = tau_l + 39.4_r8*(this%frac_loading(i)*this%non_trunk_loading/0.45_r8/10.0_r8)* & |
There was a problem hiding this comment.
Unnecessary whitespace before the & here.
| FireDuration = (SF_val_max_durat + 1.0_r8)/(1.0_r8 + SF_val_max_durat* & | ||
| exp(SF_val_durat_slope*FDI)) | ||
|
|
||
| end function FireDuration |
There was a problem hiding this comment.
end function misaligned with function.
| else | ||
| if (tree_fraction > lb_threshold) then | ||
| LengthToBreadth = (1.0_r8 + (8.729_r8* & | ||
| ((1.0_r8 -(exp(-0.03_r8*m_per_min__to__km_per_hour*effective_windspeed)))**2.155_r8))) |
There was a problem hiding this comment.
windspeed_km_hr should be used here.
There was a problem hiding this comment.
This is the part that is not bit-for-bit if I change it. Maybe there is a way to set it so that it is b4b?
| LengthToBreadth = (1.0_r8 + (8.729_r8* & | ||
| ((1.0_r8 -(exp(-0.03_r8*m_per_min__to__km_per_hour*effective_windspeed)))**2.155_r8))) | ||
| else | ||
| LengthToBreadth = (1.1_r8*((m_per_min__to__km_per_hour*effective_windspeed)**0.464_r8)) |
There was a problem hiding this comment.
windspeed_km_hr should be used here.
|
|
||
| ! fire size [m2] | ||
| fire_size = FireSize(length_to_breadth, currentPatch%ROS_back, & |
There was a problem hiding this comment.
Unnecessary spacing before & here.
There was a problem hiding this comment.
This is a style thing... maybe it's just me. I try to put the & at a specific row, but I can discontinue that
There was a problem hiding this comment.
Ah ok. I generally try not to do alignment stuff like that because it inevitably gets messed up or needs to be re-aligned, but if that's your style I won't fight you on it.
|
|
||
| ! convert to area burned per area patch per day | ||
| ! i.e., fraction of the patch burned on that day | ||
| currentPatch%frac_burnt = min(0.99_r8, area_burnt/m2_per_km2) |
There was a problem hiding this comment.
0.99 is a magic number here.
There was a problem hiding this comment.
This is from the old subroutine. I have no idea why this was put here. I'm not sure why we can't burn the entire patch, but 0.99 is seemingly arbitrary.
There was a problem hiding this comment.
For sure. See my previous comment though—it'd be good for this to be a named variable.
There was a problem hiding this comment.
yeah that works for me! though perhaps we want to add this to the list of things to scientifically validate?
|
|
||
| @Test | ||
| subroutine OptimumPackingRatio_ReturnsReasonableValues(this) | ||
| ! test that when SAV is some reasonable value, OptimumPackingRatio is correclty output |
| class(TestFireEquations), intent(inout) :: this ! test object | ||
| real(r8) :: I_r(5) ! result | ||
| integer :: i ! looping index | ||
| real(r8), parameter :: moist = 0.2_r8 ! value for moiture |
There was a problem hiding this comment.
"moiture" typo here and elsewhere
| real(r8), parameter :: fuel = 10.0_r8 ! value for fuel | ||
| real(r8), parameter :: SAV = 50.0_r8 ! value for SAV | ||
| real(r8), parameter :: MEF = 0.5_r8 ! value for MEF | ||
| ! hard-coded beta ratio values |
There was a problem hiding this comment.
This comment could use an explanation of what "beta ratio" is.
|
thanks for the review @samsrabin! Will work on suggested changes. The request to change that |
|
Ahh, that makes sense. In that case, this is such a simple change that I think it'd be fine to include it on this PR, after you've shown everything else is bit-for-bit. |
@glemieux what do you think? You had mentioned you wanted this PR to be b4b. |
Description:
I have refactored the sections of the SPITFIRE code for better clarity and more sensible (IMO) calling order. I have also moved most of the equations into helper functions for testing purposes and better modularity
Expectation of Answer Changes:
None - everything is bit for bit.
One note on this - I was unable to update this method as I would like because of an order of operations (round off) difference. I would like to update this eventually but perhaps can wait until a non-b4b PR?
Or I can change it now...
Checklist
Contributor
Integrator
Test Results:
CTSM (or) E3SM (specify which) test hash-tag:
ctsm5.3.020CTSM (or) E3SM (specify which) baseline hash-tag:
ctsm5.3.020FATES baseline hash-tag:
sci.1.80.11_api.37.0.0Test Output:
Was b4b on last check - will run again after any suggested updates.