-
Notifications
You must be signed in to change notification settings - Fork 315
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
Improve performance of CNZero #234
Comments
(From @dlawrenncar 2015-11-11) Would it be possible to setup something that systematically goes through and removes variables one at a time? Is this more efficient than manually removing variables through insight or some other systematic approach? |
(From @dlawrenncar 2015-11-17) It looks to me like there is a duplicate call to call cnveg_carbonflux_inst%SetValues( &
num_soilp, filter_soilp, 0._r8, &
num_soilc, filter_soilc, 0._r8) in CNDriverMod.F90. I commented out the first one and got bit-for-bit same answers. The impact on timing was limited, a < 10% reduction in the cost of CNZero. This is probably a 'bug'. I'll setup a branch and do more comprehensive testing to see if it can really be safely removed. I also took an overly optimistic stab at commenting out call soilbiogeochem_carbonflux_inst%SetValues( &
num_soilc, filter_soilc, 0._r8) Not surprisingly, no luck. I also found that the cost is roughly evenly distributed across the four calls to soilbiogeochem_fluxes and cnveg_fluxes for C and N. This increasingly seems to me to be something that we should pass along to John Dennis and his group to see whether or not this code can be sped up. It's just a bunch of loops. |
(From @billsacks 2016-02-29) I realized that at least some of the SetValues calls are used for two purposes (I was particularly looking at the version in CNVegCarbonFluxType): (1) zeroing fluxes for special landunits, called once in initialization; (2) zeroing fluxes for vegetated landunits, every time step. Even if we get rid of (2), we need to do (1) - meaning that, if variables are removed from SetValues, they still need to be zeroed for special landunits. (For the sake of testing, the easiest thing to do may be to initialize the variable to 0 rather than nan in initAllocate - though that's not what we want to do on the trunk.) |
(From @billsacks 2016-02-29) Here's another reason to avoid blindly zeroing out variables at the start of each time step: I believe that it will mask problems like this: Consider this driver sequencing:
In contrast, if you excluded the 'foo = 0' line, then an exact restart test would flag this as a problem, because the value of foo would differ in the first timestep after restart (or foo will be NaN in the first timestep, if it is not initialized in initCold). Although if you tried to "solve" that problem by indiscriminately adding foo to the restart file, then you have re-masked the problem. So the solution here is:
|
(From @billsacks 2016-03-15) I've been coming across more examples of fluxes (from CNVegCarbonFluxType) that fall into one of two categories:
Ideally, this accumulation would just be done in one subroutine, in which case I'd vote for moving the zeroing into that subroutine. If the accumulation is spread out in multiple subroutines (a bad practice, because it makes it hard to see what the value of the variable is at a given point in the code), then the zeroing should probably be kept where it is, to ensure that the flux is zeroed before the first accumulation. |
(From @billsacks 2016-05-23) I've been coming across more examples of states (from CNVegCarbonStateType) that are zeroed in SetValues for two reasons:
Many of these - such as the summary variables computed in the Summary routine - do not need to be explicitly zeroed in every time step. These variables (e.g., totc_col) could be zeroed once for special landunits in initialization, and not zeroed every timestep like they currently are. |
update github to make cdeps ext build an action
(Original comment in NCAR teamwork, from @dlawrenncar 2015-11-11)
CNZero routine represents about a 1/3 of total cost of biogeochemistry. It is possible that this code could either be optimized or the cost substantially reduced by removing the zeroing out of variables that do not need it.
The text was updated successfully, but these errors were encountered: