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

Groundwork luluc (Biomee) #277

Draft
wants to merge 40 commits into
base: master
Choose a base branch
from
Draft

Groundwork luluc (Biomee) #277

wants to merge 40 commits into from

Conversation

marcadella
Copy link
Collaborator

  • Reworked internal data structures as groundwork for LULUC.
  • 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.

marcadella and others added 5 commits December 12, 2024 09:25
- 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.
@marcadella marcadella requested a review from fabern December 13, 2024 11:02
@marcadella marcadella marked this pull request as ready for review December 13, 2024 12:25
Copy link
Member

@fabern fabern left a 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.

R/data.R Outdated Show resolved Hide resolved
R/run_biomee_f_bysite.R Outdated Show resolved Hide resolved
R/run_biomee_f_bysite.R Outdated Show resolved Hide resolved
@@ -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.}
Copy link
Member

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)

Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See conversation.

if (is.null(init_lu))
init_lu <- c(1.0)

# Number of LU states
Copy link
Member

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 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idem.

@stineb
Copy link
Collaborator

stineb commented Dec 16, 2024

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:

lu: refers to land unit, used for example as the index of the tile (=land unit) dimension. Respective outputs should be given for each lu.

@fabern fabern marked this pull request as draft December 16, 2024 15:02
@fabern
Copy link
Member

fabern commented Dec 16, 2024

@stineb: am I understanding this correctly that land units lu then can be up to e.g. 50 different land units / tiles at each moment in time? Furthermore, with land use change they can be created and merged, thus the output on land unit / tile level would be very unstructured.

Wouldn't output on the aggregated land use type level (as suggested by marcadella) be sufficient?

@marcadella
Copy link
Collaborator Author

marcadella commented Dec 16, 2024

@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.

@fabern
Copy link
Member

fabern commented Dec 16, 2024

a) No, I'm currently not working on any BiomeE updates.
So I don't expect any divergences in the coming month.
PR frequency is thus a matter of style/choice.

b) I'm not sure I understand then the what LULUC describes.
I am however positive, that I'm not confounding land units / tiles with cohorts:

Last time we discussed that each tile has a history.
And that there can be multiple secondary forest tiles, each with a distinct history.
Basically an additional secondary forest tile gets created whenever managed land gets abandoned. This can happen once in 2010, then another time in 2015, creating thus 2 tiles. Unless we merge all of them together, this creates a variable number of tiles. Is this land use change not part of that PR? I thought this yearly land use change is what the LULUC data describes in its transmission matrix.

I am confused now about the objective of this PR.

@marcadella
Copy link
Collaborator Author

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.

marcadella added 19 commits December 19, 2024 18:14
- 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.
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 this pull request may close these issues.

3 participants