Extremely high simulated NEP in sci.1.34.0_api.9.0.0

[yanyanchengHydro]

I did two single point simulations for C4 grass using two versions of FATES: sci.1.27.2_api.7.3.0 and sci.1.34.0_api.9.0.0, to see if the latest version can reproduce my previous simulation results. But found while GPP results are consistent between the two cases (see the figure below), the NEP results from sci.1.34.0_api.9.0.0 are way too large (5000-20000 gC/m2/day) (Figure b).

@rgknox

[jkshuman]

@yanyanchengHydro I just want to confirm that you are using the same file for these simulations. I know the file format has changed, but the common parameters are the same?

[jkshuman]

also tagging @xuchongang with this grass simulation

[ckoven]

looks like a unit error. I think there should be a * per_dt_tstep here? : https://github.com/NGEET/fates/blob/master/main/FatesHistoryInterfaceMod.F90#L1590

[yanyanchengHydro]

@yanyanchengHydro I just want to confirm that you are using the same file for these simulations. I know the file format has changed, but the common parameters are the same?

Yes, the common parameters are the same, except for the new parameter values based on you and Chonggang. I doubt it is a unit error.

[ckoven]

no there is definitely a unit error. ccohort%gpp_tstep has units of (kgC/indiv/timestep) (see here: https://github.com/NGEET/fates/blob/master/main/EDTypesMod.F90#L261) but is being added to -bc_in(s)%tot_het_resp which has (gC/m2/s) (see here: https://github.com/NGEET/fates/blob/master/main/FatesHistoryInterfaceMod.F90#L1580) and the conversion factors on that line convert from kg/indiv to g/m2 but not from /timestep to /s.

[rgknox]

I am seeing the same thing that you are @ckoven . When we construct hio_nep_si, the variable that is pointed to by "NEP", the heterotrphic respiration is in units of /s, while the gpp and autotrophic respiration is in units of /timestep. We need to divide the latter two by timestep.
This certainly looks like a bug to me. Weird that we have not identified this before. I guess we have just been looking at the components of NEP lately.

[rgknox]

This one is on me, a commit from May of 2019: https://github.com/NGEET/fates/commit/9cf09c7754cfe748cc1038b5b06cb8971ead8f2f

[rgknox]

I see a couple of work-flows here:

trivial:

0) This would be klug-ey, but we could just define a constant that matches the land-model timestep and use that to normalize gpp_tstep and resp_tstep in wrap_update_cbal. Maybe this would work for @yanyanchengHydro while we work on the other options:

short-term:

1) We pass in the time-step as an argument to wrap_update_cbal so we can normalize the gpp and autotrophic respiration rates. This will require a change in the interface. I could add this to the hydro PR.

long-term:

2) I'm noticing that we have two different routines in FatesHistoryInterfaceMod that handle the history accounting of fates variables on the short time-step: wrap_update_prod, and wrap_update_cbal. The prior is called from clm_driver => CanopyFluxes, and the latter is called clm_driver =>EDBGDynSummary (this is also later in the main call sequence). I think in the long run it would be good to consolidate these two routines, and have them called after the soil BGC calls (where EDBGDynSummary). The nutrient enabled PR shuffles these calls around, so I could include this change in with that PR.

[glemieux]

611 merged with Ryan's shortterm fix. @rgknox do you want to leave this open for the long-term fix?

[rgknox]

yeah, lets leave this open until we consolidate, or decide not to consolidate, thanks for the ref