BiomeE forcing homogenization #251

marcadella · 2024-11-11T10:14:37Z

Add steps_per_day parameter to specify the resolution of the time series.
Use same forcing as p-model (RH and soil_temp are now computed from other forcing).
Simplify implementation of soiltemp() to use a constant with rolling window, and fix a bug.
Add missing initializations affect totSeed and totNewC.
Factorize zero_diagnostic() to the start of the loop.
Split script generating drivers and computing sample output in two.
Regenerate both drivers and sample outputs.
Add integration test for BiomeE

…ng ('climate_type'). - Replace the automagic time resolution discovery by a simpler user-defined 'steps_per_day' parameter in 'params_siml'. - Fix documentation issue 'recycle' is in years, not in days. - Other minor cleaning

…ndow - Use same year padding for both yearly mean temp and soil moisture (the latter was using an unpadded mean) in 'soiltemp' - Fixed a bug in `soiltemp` where return was called instead of cycle.

- Add forcing description and units to `run_biomee_f_bysite`'s doc - Minor changes in doc

…pe from p-model's

…first year and can therefore not be used as left padding after all.

…and regenerate drivers

…ithm assumes daily samples)

…e model output.

… windows.

… OSX and windows." This reverts commit 5551fa1.

… OSX and windows." This reverts commit b095173.

… OSX and windows." This reverts commit 3ebb40f.

… OSX and windows." This reverts commit 6a2ab28.

fabern · 2024-11-12T14:30:47Z

🎉 Yay: "All checks have passed"

stineb

@marcadella thanks for this milestone PR! I left a couple of questions.

src/forcing_siterun_biomee.mod.f90

src/vegetation_biomee.mod.f90

stineb · 2024-11-13T11:04:22Z

src/biosphere_biomee.mod.f90

@@ -70,62 +64,69 @@ subroutine biosphere_annual( &
      ! Initialize vegetation tile and plant cohorts
      allocate( vegn )
      call initialize_vegn_tile( vegn )
-
-      ! Sort and relayer cohorts
-      call relayer_cohorts( vegn )


Why did you remove this here @marcadella ?

relayer_cohorts is called in initialize_vegn_tile already.

stineb · 2024-11-13T11:04:39Z

src/biosphere_biomee.mod.f90

    simu_steps = 0
+    doy = 0
+    call Zero_diagnostics( vegn )


safe to remove? @marcadella

I am 'factorizing' the diagnostics reset at the start of the loop as it is cleaner than calling the zero_diagnostics function from multiple places. I checked that this is equivalent to the previous implementation.

fabern · 2024-11-13T13:49:16Z

My thirteen comments from 2 days ago (scroll up in this thread) have not yet been addressed before merging.
@marcadella could you please go through them and consider their inclusion?
@stineb, 2 out of 13 were addressed at you: (one on renaming QMD => QMD12, and another one on storing the full example output instead of only the annual tile in rsofun::biomee_p_model_output). Could you please comment on those?

(EDIT: Okay, sorry my bad. I was unaware that the review had not been submitted.)

R/run_biomee_f_bysite.R

fabern · 2024-11-11T10:56:14Z

data-raw/generate_biomee_drivers.R

-  init_soil = list(tibble(init_soil)),
-  forcing  =list(tibble(forcing))
-)
+rad_to_ppfd <- function(rad) {


Can you please add in comments the units of the input and output for both functions (rh_to_vpd() and rad_to_ppfd())?

src/biosphere_biomee.mod.f90

data-raw/generate_biomee_output.R

fabern · 2024-11-11T12:11:18Z

src/datatypes.mod.f90

+    if (vegn%nindivs12>0.0) then
+      vegn%QMD   = sqrt(vegn%DBH12pow2 / vegn%nindivs12)
+    else
+      vegn%QMD = 0.0


@stineb Why are we not using DBH but DBH12 for QMD? Would it make sense to clarify this by renaming QMD12 ?

In practice, there is a limit to which tree size in monitored, and therefore real life QMD reflect the QMD of all the trees larger than a minimum size. Here, 12cm is assumed, but @lauramarques said that the this limit should ideally be configurable. So the limit size should not be present in the variable name.

I suggest to have it in the variable name until it is configurable. (And then change the output definition, when we include the cutoff as an input parameter.)

@stineb what is your take on this?

fabern · 2024-11-11T12:30:52Z

src/params_siml_pmodel.mod.f90

  implicit none

  private
-  public paramstype_siml, getsteering, outtype_steering
+  public paramstype_siml

  !----------------------------------------------------------------
  ! Derived type for simulation parameters
  !----------------------------------------------------------------
  type paramstype_siml


Is it correct that we have two different types of the same name (paramstype_siml) for BiomeE and P-Model? Could we either harmonize their contents (difficult...) or then distinguish them by name?

Yes, that's right. I have already factorized steering_parameters, but I agree that I would be with to rename both derived types. In addition, there are plenty of parameters that are actually not used and could be removed (ex: outputhourly, outputdaily, dist_frequency, ...)

Yes, please rename the two types paramstype_siml with their current definitions. Could you then open an issue to discuss removal of unneeded fields in these types?

fabern · 2024-11-11T12:44:45Z

src/sofunutils.mod.f90

    end if

  end function running

+  subroutine downscale(out, in, rate)


In climate science downscaling is a common term which refers to increasing the spatial (or temporal) resolution (which is the opposite to 'image downscaling'). (see https://en.wikipedia.org/wiki/Downscaling or https://climate.copernicus.eu/sites/default/files/2021-01/infosheet8.pdf)

For the sake of clarity, I suggest to rename downscale() to aggregate(). Also clarify the units of rate (it is in rows of the input array, right?). Furhter, maybe call it rather resolution instead of rate?

src/soiltemp_sitch.mod.f90

fabern · 2024-11-11T14:53:45Z

src/vegetation_biomee.mod.f90

@@ -1335,29 +1333,8 @@ subroutine relayer_cohorts( vegn )
    ! THIS CREATES WEIRD BUG: SOMETIMES ZERO SOLUTION


Is the 'WEIRD BUG' still present? Or are the modification now doing what was suggested in the removed commments?

As far as I know the implementation is working. I believe the comment was copied by mistake from below. I'll remove it.

fabern · 2024-11-11T15:16:18Z

tests/testthat/test-quantitative-validation.R

-#   # test for correctly returned values
-#   expect_equal(tolerance, 0.6768124, tolerance = 0.03)
-# })
+test_that("biomeE quantitative check (gs leuning)", {


Where do we want to do these regression checks of the numeric results? In phydro-branch I added those to test-model-runs.R instead of test-quantitative-validation.R.
I did so because the latter was earmarked to be for 'quantitative check versus observations'.
I suggest thus to:

move this to test-model-runs.R

I further suggest to structure this similarly to the tests in pydro-branch. This involves the following steps instead of using just the single example combination of driver and (reduced) output:

hardcode (a subset of) the output as reference values, since this allows to better keep track of the evolution of reference values in git (see how it is done in pydro-branch)

test a number of different flag combinations (check what would make sense for BiomeE) that activate different parts of the BiomeE code (e.g. the p-model/Leuning flag)

test not only out_annual_tile, but also the other ouputs of 'runread_biomee_f' which are not all stored in the *.rda-file.

I can move it to test-model-runs, but improving the test coverage is outside the scope of the CRAN release.

fabern · 2024-11-13T14:28:32Z

Since my previous review has been added here to this closed PR, I will now also add here the feedback on documentation you requested. Based on the published documentation, I suggest the following changes:

1. init_dates_dataframe() appears to be only internally useful. Can this be removed from the exported functions and from the documentation?
2. similarly to the biomee model: should we also add p_model_output.rda and p_model_output_vcmax25.rda ?
- If we do so, should we add an additional section for 'Output data' in addition to the existing 'Driver data' in https://geco-bern.github.io/rsofun/articles/data_format.html?
3. replace 'validation data' to 'example output data' in the title for both: https://geco-bern.github.io/rsofun/reference/biomee_gs_leuning_output.html and https://geco-bern.github.io/rsofun/reference/biomee_p_model_output.html
4. homogenize https://geco-bern.github.io/rsofun/reference/p_model_drivers_vcmax25.html and https://geco-bern.github.io/rsofun/reference/p_model_drivers.html
5. extend https://geco-bern.github.io/rsofun/reference/biomee_validation.html at least with data description but preferably also with sources (apparently this is data from CH-LAE). So that eventually the docs are similar to https://geco-bern.github.io/rsofun/reference/p_model_validation_vcmax25.html and https://geco-bern.github.io/rsofun/reference/p_model_validation.html
6. is it possible to modify the _pkgdown.yml to:
- reorder the articles (https://geco-bern.github.io/rsofun/articles/) as below:
  P-model usage
  BiomeE usage
  Data format
  Parameter calibration and cost functions
  Sensitivity analysis and calibration interpretation
- split (https://geco-bern.github.io/rsofun/reference/index.html) between data sets (drivers, output, validation) and functions?

fabern · 2024-11-13T16:15:12Z

7. be more explicit on https://geco-bern.github.io/rsofun/articles/data_format.html, e.g.

BiomeE follows the same structure of input data as P-model overall, but with some differences regarding both parameters and forcing.
Details about parameters can be obtained in the documentation: ?run_biomee_f_bysite.
The required structure of forcing data are provided by the two example data sets below. Note that 'hod' hour of day is only used when running the gs_leuning photosynthesis and not when running pmodel.
rsofun::biomee_p_model_drivers$forcing
rsofun::biomee_gs_leuning_drivers$forcing
8. is there a check on input data depending on code_method_photosynth ?
9. explain the effect of output_hourly_tile and the parameter outputhourly when running p-model in the doc of run_biomee_f_bysite, i.e. when code_method_photosynth=="pmodel" as opposed to when code_method_photosynth=="gs_leuning".

marcadella · 2024-11-13T18:46:54Z

init_dates_dataframe() is tested in test_ancillary_functions.R so I think it is meant as a utility function for the users.
Out of scope for the CRAN release.
Ok
Out of scope for the CRAN release.
Ok
Looks like they are sorted alphabetically. I don't think it is wise to hard code the order in this file (if at all possible?) as when we will add new vignettes, we will of course forget to update this file too... But I could be possible to add a number in front of the name?
More explicit about what?
Yes, the model will complain if the wrong resolution is used with the wrong photosynthesis method. Other than that, both models use the same input data.
outputhourly is not used at all...

fabern · 2024-11-14T09:23:12Z

Ok, sounds good. Some clarifications to four of the points.

'1. @stineb could you confirm that init_dates_dataframe() should be a function that is exposed to the package user? I would suggest to make it internal only since I do not see when a user would need this.
'4. I think homogenizing the documentation should be done before CRAN release.
'6. yes I think prepending a number to the title is a good, robust workaround.
'7. explicit about the different ways the drivers can be formatted for Biomee. Basically just what I had suggested in the quoted part.

marcadella · 2024-11-14T10:20:45Z

@fabern Do you have anything special in mind regarding pt4?

marcadella · 2024-11-14T10:22:42Z

Created a new issue: #253

fabern · 2024-11-14T10:33:30Z

@fabern Do you have anything special in mind regarding pt4?

That the two sites (https://geco-bern.github.io/rsofun/reference/p_model_drivers_vcmax25.html and https://geco-bern.github.io/rsofun/reference/p_model_drivers.html) show e.g. the forcing arguments in the same order and have the same description.

marcadella added 30 commits October 24, 2024 08:16

Revert changes to sofunutils

c446cd1

- Simplify the implementation of 'soiltemp' to use a fixed 30 days wi…

62f41fa

…ndow - Use same year padding for both yearly mean temp and soil moisture (the latter was using an unpadded mean) in 'soiltemp' - Fixed a bug in `soiltemp` where return was called instead of cycle.

Re-add no-padding capability to 'running' function.

bdfc11b

Fix typos

522bce9

Remove unused function

5e389b9

Fix start index in soiltemp

e4484d4

Merge branch 'fix-soil-temp' into 228-forcing-homogenization

497a9f4

Implement soil temp from air temp using soiltemp

fff9e3a

- Rename biomee forcing to match p-model's

b108e2e

- Add forcing description and units to `run_biomee_f_bysite`'s doc - Minor changes in doc

- Update data_format.Rmd vignette to include a section about biomee

f9d56df

- Fix compilation

1572eee

- Fix segfault due to myinterface for biomee being of a separate ty…

4493b7d

…pe from p-model's

Fix conversion rh -> vpd

36aa42c

Fix conversion rh -> vpd

7d1dc2b

Update biomee drivers

c2c2be8

Fix wrong implementation of soiltemp: wscal must be computed for the …

bc76785

…first year and can therefore not be used as left padding after all.

Start with non-zero C in soil pools to avoid NaN CN ratios.

956f3e1

Update drivers

35c5a73

Fix issue with day loop not incrementing properly doy

1fe43e1

Fix typo in get_steering

edff48b

Fix typo in get_steering

506f622

Fix dimension of forcing array

7bb244b

Fix driver generation issue (wrong interpretation of ppfd parameter) …

3ae4691

…and regenerate drivers

Fix biomee soiltemp computing daily from daily temperature (the algor…

3621528

…ithm assumes daily samples)

Simplify driver generation

bbdf492

Factorize get_cycleyear and steering parameters

b4b2ede

Add error message if steps_per_day parameter missing

0522899

Fix 'Transp' out variable containing a new line

de1c17d

Minor cleanup

d10d08d

marcadella added 16 commits November 11, 2024 11:52

We make sure the right drivers and data are loaded before checking th…

a87e18d

…e model output.

Use tolerance to compare output means.

b7bf3e5

Raise tolerance to 1e-4

3a8fe05

Just checking if the validation datais present as expected on OSX and…

6a2ab28

… windows.

Just checking if the validation datais present as expected on OSX and…

3ebb40f

… windows.

Just checking if the validation datais present as expected on OSX and…

b095173

… windows.

Just checking if the validation datais present as expected on OSX and…

5551fa1

… windows.

Revert "Just checking if the validation datais present as expected on…

bbd94bd

… OSX and windows." This reverts commit 5551fa1.

Revert "Just checking if the validation datais present as expected on…

fac5bd9

… OSX and windows." This reverts commit b095173.

Revert "Just checking if the validation datais present as expected on…

b54c578

… OSX and windows." This reverts commit 3ebb40f.

Revert "Just checking if the validation datais present as expected on…

82888e2

… OSX and windows." This reverts commit 6a2ab28.

Try adding printouts to locate error

babbaaf

Try adding printouts to locate error

7fd0907

Remove printouts

7d0bd97

Fix heap corruption

c09801f

Minor: fix some compiler warnings

1cae897

marcadella requested a review from stineb November 12, 2024 13:41

stineb reviewed Nov 13, 2024

View reviewed changes

stineb merged commit 69fcbe2 into master Nov 13, 2024
7 checks passed

marcadella deleted the 228-forcing-homogenization branch November 13, 2024 13:24

fabern reviewed Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BiomeE forcing homogenization #251

BiomeE forcing homogenization #251

marcadella commented Nov 11, 2024

fabern commented Nov 12, 2024

stineb left a comment

stineb Nov 13, 2024

marcadella Nov 13, 2024

stineb Nov 13, 2024

marcadella Nov 13, 2024

fabern commented Nov 13, 2024 •

edited

Loading

fabern Nov 11, 2024

fabern Nov 11, 2024

marcadella Nov 13, 2024

fabern Nov 14, 2024

fabern Nov 11, 2024

marcadella Nov 13, 2024

fabern Nov 14, 2024

fabern Nov 11, 2024

marcadella Nov 13, 2024

fabern Nov 11, 2024

marcadella Nov 13, 2024

fabern Nov 15, 2024

fabern Nov 11, 2024

marcadella Nov 13, 2024

fabern Nov 15, 2024

fabern commented Nov 13, 2024 •

edited

Loading

fabern commented Nov 13, 2024 •

edited

Loading

marcadella commented Nov 13, 2024

fabern commented Nov 14, 2024

marcadella commented Nov 14, 2024

marcadella commented Nov 14, 2024

fabern commented Nov 14, 2024 •

edited

Loading

		@@ -1335,29 +1333,8 @@ subroutine relayer_cohorts( vegn )
		! THIS CREATES WEIRD BUG: SOMETIMES ZERO SOLUTION

BiomeE forcing homogenization #251

BiomeE forcing homogenization #251

Conversation

marcadella commented Nov 11, 2024

fabern commented Nov 12, 2024

stineb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabern commented Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabern commented Nov 13, 2024 • edited Loading

fabern commented Nov 13, 2024 • edited Loading

marcadella commented Nov 13, 2024

fabern commented Nov 14, 2024

marcadella commented Nov 14, 2024

marcadella commented Nov 14, 2024

fabern commented Nov 14, 2024 • edited Loading

fabern commented Nov 13, 2024 •

edited

Loading

fabern commented Nov 13, 2024 •

edited

Loading

fabern commented Nov 13, 2024 •

edited

Loading

fabern commented Nov 14, 2024 •

edited

Loading