SIPNET Initialization Code (carbon and nitrogen pool calculations)#3683
SIPNET Initialization Code (carbon and nitrogen pool calculations)#3683mattykim06 wants to merge 2 commits intoPecanProject:developfrom
Conversation
|
|
||
| # File that will initialize SIPNET. Takes in the UniqueID(polygon), the date (year and day), | ||
| # CLASS and SUBCLASS (ex. C2), and LAI | ||
| initialize_SIPNET <- function(UniqueID, Date, Event, CLASS_SUBCLASS, LAI) { |
There was a problem hiding this comment.
planting and harvest have different columns, so should be returned as different data frames
| # Map CLASS and SUBCLASS | ||
| group_name <- get_crop_name(CLASS_SUBCLASS) | ||
|
|
||
| LAI = LAI * 10000 |
There was a problem hiding this comment.
Not following this undocumented multiplication of a dimensionless variable by an arbitrary constant
There was a problem hiding this comment.
Please use PEcAn.utils::ud_convert()
| if (Event == "planting") { | ||
| planting_params <- initialize_planting(group_name, LAI) | ||
|
|
||
| # Extract values with explicit NA handling |
There was a problem hiding this comment.
All this seems superfluous. Just put planting_params directly into the returned dataframe, no need to create all these temporary variables.
| group_name <- get_crop_name(CLASS_SUBCLASS) | ||
|
|
||
|
|
||
| val_3446 <- get_fallback_value(group_name, 3446) |
There was a problem hiding this comment.
I'd recommend using meaningful variable names, not all these undocumented codes (e.g. 3446)
There was a problem hiding this comment.
👍🏻 that will make it easier to understand the mapping.
I wonder if this a table or tables with fallback values would simplify the lookup function, and make it easier to understand and extend.
| LAI = LAI * 10000 | ||
|
|
||
| #val_601 <- get_fallback_value(group_name, trait_id = 601) | ||
| val_3441 <- get_fallback_value(group_name, trait_id = 3441) |
There was a problem hiding this comment.
separate the calculation of parameters from there use in calculating pools. Parameters should all be precalculated into a simple static lookup table based on group_name
| C_coarseRoot.fraction <- 0.5 | ||
| CN_root <- val_1055 | ||
| CN_stem <- 70 #placeholder | ||
| SLA <- (val_3115 + val_3116 + val_3117) / 3 |
There was a problem hiding this comment.
What's going on here? SLA is a single pre-calculated value, not the average of three things
| coarseroot_nitrogen = N_coarseRoot | ||
| ) | ||
|
|
||
| C_leaf <- planting_params[["leaf_carbon"]] |
There was a problem hiding this comment.
I'm not following this chunk of code and the one above it. The first seems to take the calculated pools and puts them into a list, the second seems to take those values out of the list and put them back into constants, in the end achieving nothing.
There was a problem hiding this comment.
Thanks for submitting this PR. The recent #3765 adds PEcAn.data.land::landiq_crop_mapping_codes. This can replace Class_Subclass_Mapping.R, and optionally the second (drop class_mapping.R), since those are the actual code/name lookups. Also, you are in good company. I used the same approach in #3423
For group to type, this could be merged with the metadata tables that @camalmborg prepared (and are currently in the https://github.com/ccmmf/cadwr-landuse) repository.
Also, how much of this is SIPNET-specific? Does anything belong in the SIPNET package? Does anything belong in data.land?
| @@ -0,0 +1,187 @@ | |||
| genus_to_group <- c( | |||
There was a problem hiding this comment.
could this be stored as a documented genus to groups mapping table?
| M_coarseRoot <- val_1534 | ||
| N_leaf.mass <- val_14 | ||
| CN_fineRoot <- val_2057 | ||
| C_leaf.fraction <- 0.47 |
There was a problem hiding this comment.
Are these hard-coded values available from the database? If it is necessary / expedient to have hard coded values, please cite a source.
|
@sarahkanee @mattykim06 is this still the correct PR for this work or has this code been refactored? |
4 participants
…pools)
Description
Motivation and Context
Review Time Estimate
Types of changes
Checklist: