Skip to content
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

phydro issues #218

Open
6 of 13 tasks
stineb opened this issue Aug 9, 2024 · 7 comments · May be fixed by #222
Open
6 of 13 tasks

phydro issues #218

stineb opened this issue Aug 9, 2024 · 7 comments · May be fixed by #222

Comments

@stineb
Copy link
Collaborator

stineb commented Aug 9, 2024

@jaideep777

  • Resolve compilation error on M1 Mac (may test on a different platform with rhub checks, see below):
  dlopen(/Library/Frameworks/R.framework/Versions/4.4-arm64/Resources/library/00LOCK-rsofun/00new/rsofun/libs/rsofun.so, 0x0006): symbol not found in flat namespace '___gcc_nested_func_ptr_created'
Error: loading failed
Execution halted
ERROR: loading failed

I suspect this error arises because you have contains statements inside functions and further function definitions below that. Could you move all contains statements inside functions (only allowed at top level in module) and specify the functions outside?

  • Make sure that ET can be simulated with three options (a) SPLASH as done previously with AET considering diurnal cycle of net radiation and partial supply rate limitation (b) fully coupled to the atmosphere (T = 1.6 Gs D); (c) Penman-Monteith. Specify options through drivers$params_siml.
  • Run devtools::check() and resolve all issues.
  • Run rhub checks, see here.
  • vignettes folder must contain only .Rmd files. Scripts go into analysis/. Function definitions go into R/. Shell scripts go into src/
  • Build website locally (pkgdown::build_site()) and clean it up - All .Rmd files in vignettes/ will appear as 'Articles'. This will be public-facing. Make it nice.
  • Make sure that all FluxDataKit-related code is not in {rsofun}.
  • In all vignettes and scripts, do not read repo-external data. All data used by vignettes must be contained in the repo and scripts for data generation must be in data-raw/.
  • Some functions defined in module md_photosynth_phydro are duplicated as they are already in module md_waterbal. Please remove them from md_photosynth_phydro.
  • Could Penman-Montheith-specific code go into a md_waterbal_pml, see here? (maybe no so important)
  • Avoid stop, print, and write statements in Fortran code (except for testing, but comment out upon commit).
  • Options (ET diffusion vs PM) to be set through drivers object (params_siml). I see it's in there but I see it's also set in md_photosynth_phydro.
  • Can you point me to how I use the Gs-P-model-coupled ET instead of SPLASH AET? Maybe in a separate vignette, that shows the two options like for GPP but for ET and also at FR-Pue?

I suspect that a clear separation between the model and the analysis done for your research @jaideep777 is needed. Keep only the very minimum in the {rsofun} repo such that you achieve a documentation of a very simplified example. Best is to use the data already contained in the repo (flux data for FR-Pue). All that goes beyond that and which is specific to what you did for the model calibration and evaluation on a larger set of sites should go in a separate (personal analysis) repository. Remember that our plan is to have {rsofun} on CRAN...

@stineb
Copy link
Collaborator Author

stineb commented Aug 14, 2024

Points resolved (not yet all) as per PR #219

@stineb
Copy link
Collaborator Author

stineb commented Aug 14, 2024

Build of sensititivity_analysis.Rmd now fails with error:

> morrisOut.df <- data.frame(
+   parameter = names(par_cal_best),
+   mu.star = apply(abs(morrisOut$ee), 2, mean, na.rm = T),
+   sigma = apply(morrisOut$ee, 2, sd, na.rm = T)
+ ) %>%
+   arrange( mu.star )
Error in data.frame(parameter = names(par_cal_best), mu.star = apply(abs(morrisOut$ee),  : 
  arguments imply differing number of rows: 11, 10

Should run this again but if fails, too:

> morrisOut <- sensitivity::morris(
+   model = morris_setup$posterior$density,
+   factors = names(par_cal_best), 
+   r = 1000, 
+   design = list(type = "oat", levels = 20, grid.jump = 3), 
+   binf = par_cal_min, 
+   bsup = par_cal_max, 
+   scale = TRUE)
Warning in dunif(x, min = lower, max = upper, log = T) : NaNs produced
Error in if (out == Inf) stop("Inf encountered in prior") : 
  missing value where TRUE/FALSE needed
  • Has number of model parameter or their names changed with the PR?
  • Should test separately: cost_likelihood_pmodel()

@stineb
Copy link
Collaborator Author

stineb commented Aug 14, 2024

Can PM be used in combination with P-model-Gs? There is code that is now commented but may have to be un-commented and completed?
In gpp_pmodel.mod.f90:

      if (use_pml) then
        ! We plug Pmodel/Phydro-derived gs into the PM equation to calculate T (note this is uncoupled PM-transpiration)
        ! use PFT-specific gs for this calculation: tile_fluxes(lu)%plant(pft)%gs_accl

        ! TODO: Fill this in
        ! tile_fluxes(lu)%plant(pft)%dtransp = PM_EQUATION(tile_fluxes(lu)%plant(pft)%gs_accl)

The argument using_pml is not used in subroutines of waterbal_splash.mod.f90.
@jaideep777

@jaideep777
Copy link

jaideep777 commented Aug 14, 2024

Yes, as mentioned in the comment, this is just a placeholder where the PM equation should go. It's not coded in yet. Just uncommenting won't do, because PM_EQUATION doesn't exist. It'll have to be coded.

All the PM equation calculations should be done at the PFT level, hence they are in gpp_pmodel rather than waterbal_splash

@stineb
Copy link
Collaborator Author

stineb commented Aug 14, 2024

I think the PM should be applied at the LU-level (land unit-level), not PFT-level. I suggest to use the fractional plant coverage-weighted average Gs and calculate PML-ET in the waterbal module (it's fine to duplicate code that is already in photosynth_phydro.mod.f90 for now).

@jaideep777
Copy link

Isn't the whole point of PM to distinguish surface properties of different vegetation types? If you want to do this only for inter-cell differences, then yes, the entire code chunk with the T calculation can be moved to waterbal_splash

@stineb
Copy link
Collaborator Author

stineb commented Aug 14, 2024

The gridcell-average (or land unit-average) surface properties can be used. The model structure is adopted from LPX (and other similar models) where all PFTs have access to the same soil water and the soil water balance is done at the gridcell-level (or land-unit level). The idea is that PFTs occur tightly interspersed and are not separated into disjunct tiles. The canopy exchanges with the atmosphere and represents a mix of PFTs.
I see that there will be an inconsistency when transpiration is calculated per PFT following diffusion, and then total canopy-transpiration is calculated at the gridcell-level follwoing PML. They won't add up.

@fabern fabern linked a pull request Sep 19, 2024 that will close this issue
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants