Implementation of eq 7 from Kumarathunge et al., 2019

[maloan]

The functions calculating photosynthetic temperature responses (calc_ftemp_inst_vcmax and calc_ftemp_inst_jmax) have been updated following the equation 7 in Kumarathunge et al. (2019).
This implementation introduces a "home temperature" (tchome), defined as the long-term mean maximum temperature of the warmest month, which acts as a reference for acclimation.

[fabern]

I suggest we keep this open and simply transform it to draft status.
(So I remember something will be incoming...)

[fabern]

Thanks @maloan. This looks like a proper PR. Very nice!
I left a few minor comments throughout.
Once addressed, I suggst we can merge this into main.

Also thanks for addressing the CI checks, including the R CMD check.

[fabern]

Why changing here the outcommented vcmax to jmax? Do we need the outcommented part anymore? If not, better delete it.

[fabern]

Conceptually tc_home is a site-level property similar to whc. To reflect this I suggest to save this into site_info$tc_home.

We are already doing this with params_siml, so I see no issue in extending the passed-in arguments.

[fabern]

Is the removal of these lines wanted? Or was this automatically done by your editor.
I suggest to keep the changes by this PR to a minimal.

I.e. unless strongly wanted, remove these whitespace changes. I don't want to end up in a situation where the editor style settings of one contributor fight against those of antoher contributor.

[fabern]

Could you check why the drivers show as modified?
I think this PR doesn't add anything to the API, right? Since you're computing tchome within run_pmodel_f_bysite the driver should not need changes.

If however, the driver does require changes, that means a breaking API change, and we should then also update documentation etc.

[fabern]

I suggest renaming to tc_leaf, tc_growth, and tc_home (here and throughout the code), to be consistent with how it is done in BiomeE.

[fabern]

This output file should not have been modified by your change, should it?

[fabern]

Okay, no the RDA file appears to be the same.
Then that is okay.

Note, I checked this by downloading the previous and current RDA files into two subfolders "master" and "updated" to check their equivalence with testthat::expect_equal().

# load all RDA files into two different environments
# to read on environments: https://adv-r.hadley.nz/environments.html
master <- rlang::env()
update <- rlang::env()
master_rda_files <- list.files("master/", full.names = TRUE)
updated_rda_files <- list.files("updated/", full.names = TRUE)
for (rda_file in master_rda_files){load(rda_file, envir = master)}
for (rda_file in updated_rda_files){load(rda_file, envir = update)}

library(testthat)
testthat::expect_equal(master[['biomee_gs_leuning_drivers']],
                       update[['biomee_gs_leuning_drivers']])

# variables_to_test <- ls(envir = master)
# for(varname in variables_to_test){
#   testthat::expect_equal(master[[varname]],
#                          update[[varname]])
# }
# dput(variables_to_test)
testthat::expect_equal(master[["biomee_gs_leuning_output"]],update[["biomee_gs_leuning_output"]])

tibble(master[["biomee_gs_leuning_output"]] |> unnest_wider(data))
tibble(update[["biomee_gs_leuning_output"]] |> unnest_wider(data))

tibble(master[["biomee_gs_leuning_output"]] |> unnest_wider(data) |> unnest(output_daily_tile))
tibble(update[["biomee_gs_leuning_output"]] |> unnest_wider(data) |> unnest(output_daily_tile))

tibble(master[["biomee_gs_leuning_output"]] |> unnest_wider(data) |> unnest(output_annual_tile))
tibble(update[["biomee_gs_leuning_output"]] |> unnest_wider(data) |> unnest(output_annual_tile))

tibble(master[["biomee_gs_leuning_output"]] |> unnest_wider(data) |> unnest(output_annual_cohorts))
tibble(update[["biomee_gs_leuning_output"]] |> unnest_wider(data) |> unnest(output_annual_cohorts))

[fabern]

After code review and since all tests pass, this looks good for squashing and merging.

🎉

[fabern]

This had been undone for the revision of the manuscript.
However, now we decided to include this and thus combine it with PR #296