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

Cluster-level Residues Realization #585

Merged
merged 13 commits into from
Sep 23, 2023
Merged

Conversation

caviddhen
Copy link
Contributor

@caviddhen caviddhen commented Aug 8, 2023

🐦 Description of this PR 🐦

Add a cluster-level residues realization of reduced complexity. This realization allows for tracking the cluster-level production of agricultural residues via the new variable v18_prod_res. Other fates of residues (i.e. recycling and burning), as well as below-ground production, still happen at regional level, in order to reduce runtime.

🔧 Checklist for PR creator 🔧

  • Label pull request from the label list.

    • Low risk: Simple bugfixes (missing files, updated documentation, typos) or changes in start or output scripts
    • Medium risk: Uncritical changes in the model core (e.g. moderate modifications in non-default realizations)
    • High risk: Critical changes in model core or default settings (e.g. changing a model default or adjusting a core mechanic in the model)
  • Self-review own code

    • No hard coded numbers and cluster/country/region names.
    • The new code doesn't contain declared but unused parameters or variables.
    • magpie4 R library has been updated accordingly and backwards compatible where necessary.
    • scenario_config.csv has been updated accordingly (important if default.cfg has been updated)
  • Document changes

    • Add changes to CHANGELOG.md
    • Where relevant, put In-code documentation comments
    • Properly address updates in interfaces in the module documentations
    • run goxygen::goxygen() and verify the modified code is properly documented
  • Perform test runs

    • Low risk:
      • Run a compilation check via Rscript start.R --> "compilation check"
    • Medium risk:
      • Run test runs via Rscript start.R --> "test runs"
      • Check logs for errors/warnings
    • High risk:
      • Run test runs via Rscript start.R --> "test runs"
      • Check logs for errors/warnings
      • Default run from the PR target branch for comparison
      • Provide relevant comparison plots (land-use, emissions, food prices, land-use intensity,...)

📉 Performance changes 📈

  • Current develop branch default : 0.45 mins
  • This PR's default : 0.46 mins

🚨 Checklist for reviewer 🚨

  • PR is labeled correctly
  • Code changes look reasonable
    • No hard coded numbers and cluster/country/region names.
    • No unnecessary increase in module interfaces
    • model behavior/performance is satisfactory.
  • Changes are properly documented
    • CHANGELOG is updated correctly
    • Updates in interfaces have been properly addressed in the module documentations
    • In-code documentation looks appropriate
  • content review done (at least 1)
  • RSE review done (at least 1)

Copy link
Contributor

@pfuehrlich-pik pfuehrlich-pik left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this impressive work and major contribution! The code is well documented, so even though I don't know much at all about how magpie's gams code works I could follow rather well! The one bigger thing I'd suggest though is not using the word "cell" or "cellular" anymore, as it means cluster in some cases and gridcell in others. In magpie's gams code, cell always means cluster (as Patrick told me), so I'd suggest always using the words "cluster" or "cluster-level" (as you already do in some cases) instead.

main.gms Outdated
@@ -221,7 +221,7 @@ $offlisting

$setglobal c_timesteps coup2100
$setglobal c_past till_2010
$setglobal c_title default
$setglobal c_title testCellRes7_flexcell
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you really intend to commit your changes to main.gms?


positive variables
v18_prod_res(j,kres) Cluster-level production of residues (mio. tDM)
v18_res_biomass_ag_cell(j,kcr) Production of aboveground residues in each cluster (mio. tDM)
Copy link
Contributor

Choose a reason for hiding this comment

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

I really think we should stop calling clusters cells, how about this variable name?

Suggested change
v18_res_biomass_ag_cell(j,kcr) Production of aboveground residues in each cluster (mio. tDM)
v18_res_biomass_ag_cluster(j,kcr) Production of aboveground residues in each cluster (mio. tDM)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe that means that the whole realization name than should be changed?
According to that it might make sense to be a bit more specific as only the removals are on cluster/cell level?
Sorry I know renaming always costs some efforts ^^'

*#################### R SECTION START (OUTPUT DECLARATIONS) ####################
parameters
ov18_prod_res(t,j,kres,type) Cellular production of residues (mio. tDM)
ov18_res_biomass_ag_cell(t,j,kcr,type) Production of aboveground residues in each cell (mio. tDM)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, now cell no longer means cluster but it actually means cellular (as in our 59k/67k cells)? Or do you actually mean cluster again?

oq18_prod_res_bg_cell(t,i,kcr,dm_nr,type) Cellular production constraint of belowground residues (mio. tDM)
oq18_regional_removals(t,i,kcr,attributes,type) Calculation of regional level removals (mio. tX)
oq18_res_field_balance(t,i,kcr,attributes,type) Calculation of the residues amount recycled to soils (mio. tDM)
oq18_cell_field_constraint(t,j,kres,type) Make sure the amount removed in any cell does not exceet the amoutn available
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
oq18_cell_field_constraint(t,j,kres,type) Make sure the amount removed in any cell does not exceet the amoutn available
oq18_cell_field_constraint(t,j,kres,type) Make sure the amount removed in any cell does not exceed the amount available

oq18_res_field_balance(t,i,kcr,attributes,type) Calculation of the residues amount recycled to soils (mio. tDM)
oq18_cell_field_constraint(t,j,kres,type) Make sure the amount removed in any cell does not exceet the amoutn available
oq18_res_field_burn(t,i,kcr,attributes,type) Fixing of the residues amount burned in a region in respective attribute units DM GJ Nr P K WM C (mio. tX)
oq18_translate(t,j,kres,attributes,type) Transformation of the multiple crop residues into supply balance crop redisues in respective attribute units DM GJ Nr P K WM C (mio. tX)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
oq18_translate(t,j,kres,attributes,type) Transformation of the multiple crop residues into supply balance crop redisues in respective attribute units DM GJ Nr P K WM C (mio. tX)
oq18_translate(t,j,kres,attributes,type) Transformation of the multiple crop residues into supply balance crop residues in respective attribute units DM GJ Nr P K WM C (mio. tX)

@@ -0,0 +1,2 @@
name,type,reason
vm_prod,input,questionnaire
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "questionnaire" mean here?

# | Contact: [email protected]

# ------------------------------------------------
# description: start run with default.cfg settings
Copy link
Contributor

@pfuehrlich-pik pfuehrlich-pik Aug 9, 2023

Choose a reason for hiding this comment

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

This seems to be copied from default.R, please adapt.

# Source default cfg. This loads the object "cfg" in R environment
source("config/default.cfg")

#start MAgPIE run
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#start MAgPIE run

#start MAgPIE run
cfg$title <- "testCellRes2_default_noNewEquation"

#start MAgPIE run
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#start MAgPIE run

#start MAgPIE run

cfg$gms$residues <- "flexreg_apr16"
#start_run(cfg, codeCheck = TRUE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#start_run(cfg, codeCheck = TRUE)

Copy link
Member

@k4rst3ns k4rst3ns left a comment

Choose a reason for hiding this comment

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

Equation look good to me. Realization despription highlights that residue production/removal is on cluster level, but changelog as well as naming might be changed to a clearer name, as it might be that upcoming realizations might be as well on cluster level having other features (e.g. recycling on cluster level).

CHANGELOG.md Outdated
@@ -11,7 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
-

### added
-
- **18_residues** Included cluster-level residue realization
Copy link
Member

Choose a reason for hiding this comment

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

maybe more specific what variables are on cluster level?

vm_res_biomass_ag(i,kcr,attributes) Production of aboveground residues in each region (mio. tDM)
vm_res_biomass_bg(i,kcr,dm_nr) Production of belowground residues in each region (mio. tDM)

v18_res_ag_removal(j,kcr,attributes) Removal of crop residues in respective attribute units DM GJ Nr P K WM C (mio. tX)
Copy link
Member

Choose a reason for hiding this comment

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

Will that be an interface soon?
If so maybe already now make it an interface and think about how to fill it for the reg module realization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v18_prod_res will be the interface, and this one already has an equation distributing it felxibly added to the reg module realization.

Copy link
Contributor

@pfuehrlich-pik pfuehrlich-pik left a comment

Choose a reason for hiding this comment

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

Looks good, just some typos and one important thing: Please double check that the correct realization name is used everywhere. While you're at it I think it would be a good idea also rename once again to flexcluster_jul23 to be consistent with the 3-letter-month used for all other realizations.

main.gms Outdated
@@ -241,7 +241,7 @@ $setglobal food anthro_iso_jun22
$setglobal demand sector_may15
$setglobal production flexreg_apr16

$setglobal residues flexreg_apr16
$setglobal residues flexcell_july23
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$setglobal residues flexcell_july23
$setglobal residues flexcluster_july23

Copy link
Contributor

@pfuehrlich-pik pfuehrlich-pik Sep 22, 2023

Choose a reason for hiding this comment

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

For consistency the realization should probably be renamed like this (jul instead of july)

Suggested change
$setglobal residues flexcell_july23
$setglobal residues flexcluster_jul23


positive variables
v18_prod_res(j,kres) Cluster-level production of residues (mio. tDM)
v18_res_biomass_ag_clust(j,kcr) Production of aboveground residues in each cluster (mio. tDM)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
v18_res_biomass_ag_clust(j,kcr) Production of aboveground residues in each cluster (mio. tDM)
v18_res_biomass_ag_clust(j,kcr) Production of aboveground residues in each cluster (mio. tDM)

;

parameters
i18_res_use_burn(t_all,dev18,kcr) Share of residues burned on field (1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
i18_res_use_burn(t_all,dev18,kcr) Share of residues burned on field (1)
i18_res_use_burn(t_all,dev18,kcr) Share of residues burned on field (1)

oq18_prod_res_ag_reg(t,i,kcr,attributes,type) Regional production constraint of aboveground residues (mio. tDM)
oq18_prod_res_bg_cell(t,i,kcr,dm_nr,type) Cluster-level production constraint of belowground residues (mio. tDM)
oq18_res_field_balance(t,i,kcr,attributes,type) Calculation of the residues amount recycled to soils (mio. tDM)
oq18_cell_field_constraint(t,j,kres,type) Make sure the amount removed in any cell does not exceet the amoutn available
Copy link
Contributor

Choose a reason for hiding this comment

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

It probably makes sense, but this is the only parameter without a unit in the description.

oq18_prod_res_ag_reg(t,i,kcr,attributes,type) Regional production constraint of aboveground residues (mio. tDM)
oq18_prod_res_bg_cell(t,i,kcr,dm_nr,type) Cluster-level production constraint of belowground residues (mio. tDM)
oq18_res_field_balance(t,i,kcr,attributes,type) Calculation of the residues amount recycled to soils (mio. tDM)
oq18_cell_field_constraint(t,j,kres,type) Make sure the amount removed in any cell does not exceet the amoutn available
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
oq18_cell_field_constraint(t,j,kres,type) Make sure the amount removed in any cell does not exceet the amoutn available
oq18_cell_field_constraint(t,j,kres,type) Make sure the amount removed in any cell does not exceed the amount available

* vm_res_biomass_ag(i2,kcr,attributes);


*' While the residue biomass is estiamted with a crop-specific nutrient
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*' While the residue biomass is estiamted with a crop-specific nutrient
*' While the residue biomass is estimated with a crop-specific nutrient



*####################### R SECTION START (PHASES) ##############################
$Ifi "%phase%" == "sets" $include "./modules/18_residues/flexcell_july23/sets.gms"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please have another look that "flexcell_july23" is renamed everywhere

pk18(npk) subset of npk containing P and K nutrients
/p, k/

dev18 country develoment indicator
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dev18 country develoment indicator
dev18 country development indicator

It might make sense to install a plugin into your editor that checks for typos 😉

@caviddhen caviddhen merged commit 4834073 into magpiemodel:develop Sep 23, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants