Remove or reduce duplication in Photosynthesis code for Plant Hydraulic Stress

[ekluzek]

Erik Kluzek < erik > - 2016-06-24 14:09:30 -0600 Bugzilla Id: 2323 Bugzilla CC: andre, dlawren, oleson, rfisher, sacks,

clm4_5_8_r182 brings in plant hydraulic stress (PHS) as an option to the code. The older version needs to stay in place as well to support clm4_5 answers. The new option is brought in largely by duplicating the code in PhotosynthesisMod.F90 for PHS. Almost all subroutines have a PHS version and a non-PHS version. The effects of at least six namelist control items is also reproduced: lnc_opt, reduce_dayl_factor, vcmax_opt, leafresp_method, leafacc, and light_inhibit. Thus any testing for these control items must be duplicated for both PHS on and off. And the size of the file more than doubled from 2k to 2.5k.

This duplication leads to double maintenance of code where changes in the PHS section of the code has to be duplicated in the non-PHS section of the code. It also leads to bugs where a change is made on one side and not the other.

Because, there is so much code duplication this can be managed better by breaking it up into smaller segments that can be called by both PHS and non-PHS versions. The top section of Photosynthesis is almost identically duplicated in PhotosyntesisHydraulicStress until the loop through canopy layers above snow. The sections after that are also duplicated with the PHS setting both sun and shade. I think the duplication can be removed if it's broken up into three parts by three loops -- calculate_respiration loop, leaf-level-photosynthesis-and-stomotal-conductance, and canopy-photosynthesis-and-stomotal-conductance. The second section includes the call to hybrid or hybrid_PHS.

Other ways of handling the duplication are to set magic numbers in one place as parameters in the code, and to break up the namelist control items into small subroutines/functions that can be called in either branch. Thus these control items are tested whether called from the PHS branch or the non-PHS branch. At least some of these methods should be used to reduce the code duplication.

[ekluzek]

Erik Kluzek < erik > - 2016-06-24 14:34:37 -0600

Also the statement functions are reproduced. The statement functions need to be removed and the module versions used. We'll need to check that this doesn't change answers for PHS on or off.

[djk2120]

I shared some thoughts on this today at the CLM meeting.

The CLM4.5 code structure was not amenable to PHS:

• PHS has to solve sunlit and shaded photosynthesis simultaneously, whereas SMS solves them in sequence
• SMS can calculate BTRAN in advance of all of the photosynthesis equations, whereas the PHS version of BTRAN is coupled to transpiration

As a result, I basically forked the photosynthesis code and implemented the necessary changes to accommodate PHS. But while PHS can't function particularly well using the SMS code structure, I think we could relatively easily accommodate the SMS functionality within the PHS code structure. At which point we could excise large portions of the PhotosynthesisMod code. This would hopefully allow for the necessary backwards compatibility and avoid the potential divergence in other aspects of the photosynthesis code base, especially if the NWP community develops things on the SMS side.

I have to admit that some of the exact things that Erik is referencing are over my head, so when we undertake this refactor, I'll want to check in to make sure that I have an appropriate list of objectives that will need to be resolved.

[billsacks]

Thanks a lot for sharing these thoughts, @djk2120 . I haven't looked closely, but I have a feeling that many of the suggestions of @ekluzek would be superseded by the bigger overhaul that you have in mind.

[djk2120]

Adding some comments from Erik from another PR, which could be addressed in work to resolve this issue:

" In src/biogeophys/PhotosynthesisMod.F90:

@@ -4380,6 +4406,14 @@ subroutine calcstress(p,c,x,bsun,bsha,gb_mol,gs_mol_sun,gs_mol_sha,qsatl,qaf, & if (soilflux<0._r8) soilflux = 0._r8 qflx_tran_veg(p) = soilflux endif !save predawn vegwp g = patch%gridcell(p) if (night .and. get_local_time(grc%londeg(g))<(isecspday/2)) then vegwp_pd(p,:) = x

This isn't your fault, but the "x" array is a bad generic name to use. This predates your work, but it would be better if if was vegwp_old or some better name. I'll let you decide if you are up to changing it or not.

It's also assigning the whole array of "x", so a nitpick is that it would be clearer that it's an array assignment if it was

vegwp_pd(p,:) = x(:) "

[ekluzek]

I think a good candidate for becoming a subroutine is leaf photosynthesis. With the section of code starting at the setting of "cs" CO2 partial pressure at leaf surface (Pa), to the setting of gs_mol and would contain the if statements for Ball-Berry verses Medyln photosynthesis. This would be called once for non-PHS in ci_func and twice (once for sunlit and once for shaded) in ci_func_PHS. The one difference between the PHS and non-PHS portions for this section of the code is that bbb(p) would be sent in for non-PHS while max(bsha*bbb(p),1._r8)) would be sent in for PHS (for the Ball-Berry formulation). This would encapsulate leaf photosynthesis for the code that's replicated, include the main photosynthesis type switch, and make the ci_func subroutines much smaller while allowing some of the differences between PHS and non-PHS outside of the shared function.