-
Notifications
You must be signed in to change notification settings - Fork 29
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
Groundwork luluc (Biomee) #277
base: master
Are you sure you want to change the base?
Conversation
- Only external change is the addition of `lu_fraction` in the output (non-breaking as it is appended in last position). - Fixed a bug where cohort output were not properly numbered for multi-year simulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @marcadella for this groundwork.
As discussed over lunch, I think we should clarify at what level in the code we are dealing with the land use types (currently 4 of them) and at what level we are dealing with the land units (can be many more since we have multiple instances of secondary forest). I suggest to talk about these naming conventions (and output) in a common meeting with everybody involved. (Easier than online.)
I'd argue that the naming in the code should be modified to facilitate this distinction. I.e. we should refrain from using _lu
for variable naming, but rather favor _tile
or _landuse
if needed.
In below remarks
- I highlighted some of the places where I stumbled over this terminology.
- and marked various other small things.
@@ -179,6 +178,7 @@ | |||
#' \item{m_turnover}{Continuous biomass turnover (kg C m\eqn{^{-2}} year\eqn{^{-1}}).} | |||
#' \item{c_turnover_time}{Carbon turnover rate, calculated as the ratio | |||
#' between plant biomass and NPP (year\eqn{^{-1}}).} | |||
#' \item{lu_fraction}{Fraction of the area covered by this land unit.} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused about naming. Should we read lu
as 'land use' (primary, secondary, crop, pasture, ...) or 'land unit' (i.e up to e.g. 50 tiles per grid cell, e.g. 1 primary, 1 crop, 1 pasture and up to 47 secondary)? Probably we should discuss naming and outputs once together in a physical meeting.
My understanding:
- tile = land_unit: up to 50, i.e. we can/could have up to 50 outputs
output_daily_tile
- LU = land_use: exactly 4, i.e. we would have exactly 4 outputs
output_daily_lu
If this is under output_daily_tile
than we should call it tile_fraction
and keep up to 50 (non-aggregated) outputs.
Otherwise one needs to rename output_daily_lu
if we want to provide aggregated variables for each land use. (To be discussed in relation to #268 (comment) and #272)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment from the main PR: actually tile level can easily be more than 50 due to creation and merging of tiles with land use change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See conversation.
R/run_biomee_f_bysite.R
Outdated
if (is.null(init_lu)) | ||
init_lu <- c(1.0) | ||
|
||
# Number of LU states |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above to line 181. naming potentially needs to be reconsidered throughout.
Will it be init_lu
or init_tile
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idem.
I think it would be best if @marcadella advances the development of this branch, we give inputs if he asks for it, and we then take stock together once the first milestone is achieved. @marcadella please let us know when you're there. In case it's urgent:
|
@stineb: am I understanding this correctly that land units Wouldn't output on the aggregated land use type level (as suggested by marcadella) be sufficient? |
@stineb The reason I am pushing to master is because Fabian an myself are both working on the same pieces of code (in R). If I code all LULUC and merge in one month time, the discrepancy between Fabian's codebase and mine will be so great that it will be a nightmare to merge. Therefore, I prefer to push often to master (I always ensure that the code is backward compatible and all the tests are passing so my changes basically do not alter the programs output). Note that once the R layer is done, I can implement the Fortran code in a branch without the need to merge to Master until it is done as I am mostly the only one working with Fortran right now. @fabern You are mixing up cohort and land unit. Each land unit (LU) is a BiomeE tile associated with a real number specifying the area fraction occupied by this LU, and a definition of the land use that applies on this tile. Each tile contains up to 50 cohorts, but the concept of cohort is an implementation detail of a Biomee tile which LULUC does not care about (except during initialization where the user defines which PFT grows on which LU). LULUC has not concept of 'tile' (only LU). But it is nonetheless useful for us to talk about 'tile' when referring to Biomee before any LULUC implementation. Note that there are different categories of land use (managed vs unmanaged), each containing sub-categories (crop, pasture, ...), but the implementation makes no assumption about the categories or the number of LUs for each. All these details are configured by the driver. |
a) No, I'm currently not working on any BiomeE updates. b) I'm not sure I understand then the what LULUC describes. Last time we discussed that each tile has a history. I am confused now about the objective of this PR. |
I think I confused everyone with this PR, my bad. It should be considered as work in progress. I'll continue the implementation and ask for a review again once I am a bit further in the implementation. |
Strict check on biomee drivers
…d regenerate biomee outputs.
- factor simulation state (known otherwise as steering) from myinterface. - Remove unused code - Seems to break slightly Biomee p-model. Needs investigation day by day.
- Rename files to match current conversion (.mod) - Remove biomee-specific constants from params_core and remove un-needed dependencies
…eters (bad practice) and not actually doing much anyways (simply capping underLAImax and LAImax after year 1). Breaking change: The capping is now performed at year 0 unconditionally.
lu_fraction
in the output (non-breaking as it is appended in last position).