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

New module realizations of 30_crop that introduce fallow land and incentive-based crop rotations #398

Merged
merged 55 commits into from
Jun 21, 2022

Conversation

bodirsky
Copy link
Member

@bodirsky bodirsky commented May 26, 2022

🐦 Purpose of this PR 🐦

  • New module realizations of 30_crop that introduce fallow land and incentive-based crop rotations

fallow land in 1995
grafik
fallow land in 2020
grafik
fallow land in 2050
grafik
fallow land in 2050 with fallow subsidy
grafik

🔧 Checklist for PR creator 🔧

  • Medium risk : New realization
  • Providing additional information based on PR label
  • Medium risk : Default run based on the current version of the fork from which PR is made
    (still needs to be repeated with merge from PR in between)
  • 📉 Performance loss/gain from current default behavior 📈

grafik
actually not sure why this runtime figure shows longer runtimes...

If i check in runstatistics, I get the following:

  • Old default: 36.03299 mins
  • Current develop branch default : 39.57953 mins
  • This PR's new module realization: 36.00311 mins
  • [ x] Added changes to CHANGELOG.md
  • [ x] Compilation check (model starts without compilation errors - use gams main.gms action=c in model folder for testing).
  • [ x] No hard coded numbers and cluster/country/region names.
  • [ x] The new code doesn't contain declared but unused parameters or variables.
  • [ x] Where relevant, In-code comments added including documentation comments.
  • [ x] Made sure that documentation created with goxygen is okay (use goxygen::goxygen() for testing).
  • [ X] Changes to magpie4 R library for post processing of model output (ideally backward compatible).
  • [ X] Self-review of my own code.

⚠️ Additional notes or warnings ⚠️

NA

🚨 Checklist for RSE reviewer 🚨

  • PR is labeled correctly.
  • CHANGELOG is updated correctly
  • No hard coded numbers and cluster/country/region names.
  • No unnecessary increase in module interfaces
  • All required updates in interfaces (if any) have been properly adressed in the module contracts
  • In-code comments and documentation comments are satisfactory.
  • model behavior/performance is satisfactory.
  • Requested changes (if any) were applied correctly

🚨 Checklist for MAgPIE reviewer 🚨

  • PR is labeled correctly.
  • CHANGELOG is updated correctly
  • No hard coded numbers and cluster/country/region names.
  • Changes to the model are scientifically sound
  • In-code comments and documentation comments are satisfactory.
  • model behavior/performance is satisfactory.
  • Requested changes (if any) were applied correctly

…igation to reduce complexity of optimization)
…develop

# Conflicts:
#	config/default.cfg
…develop

# Conflicts:
#	config/default.cfg
#	modules/56_ghg_policy/price_jan20/sets.gms
bugfix in roation equations
recalibration of land expansion
adding fsdp merge script
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
modules/15_food/anthropometrics_jan18/preloop.gms Outdated Show resolved Hide resolved
modules/30_crop/penalty_apr22/equations.gms Outdated Show resolved Hide resolved
modules/30_crop/penalty_apr22/equations.gms Outdated Show resolved Hide resolved
modules/30_crop/rotation_apr22/equations.gms Outdated Show resolved Hide resolved
modules/50_nr_soil_budget/exoeff_aug16/equations.gms Outdated Show resolved Hide resolved
scripts/output/extra/disaggregation.R Outdated Show resolved Hide resolved
scripts/output/extra/disaggregation.R Show resolved Hide resolved
scripts/output/projects/fsdp_reporting_reg.R Outdated Show resolved Hide resolved
Copy link
Contributor

@pvjeetze pvjeetze left a comment

Choose a reason for hiding this comment

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

Quite amazing stuff, also considering the results you presented in the magpie meeting. I have two main points for improvement. The first one is more conceptual with regard to getting the terminology of set aside/fallow right in comparison to the older implementation of giving space for other land classes to increase compositional heterogeneity of land classes within cells. I've made suggestions how to rework the terminology in the comments.
The second point is with regard to the biodiversity stock calculations. I think the equations for perennial cropland need to be revisisted. Otherwise I just found some minor typos etc.

config/default.cfg Outdated Show resolved Hide resolved
config/default.cfg Outdated Show resolved Hide resolved
modules/30_crop/penalty_apr22/equations.gms Outdated Show resolved Hide resolved
modules/30_crop/penalty_apr22/equations.gms Outdated Show resolved Hide resolved
modules/30_crop/penalty_apr22/input.gms Outdated Show resolved Hide resolved
modules/30_crop/rotation_apr22/equations.gms Outdated Show resolved Hide resolved
modules/30_crop/rotation_apr22/input.gms Outdated Show resolved Hide resolved
modules/30_crop/rotation_apr22/input/files Outdated Show resolved Hide resolved
modules/30_crop/rotation_apr22/preloop.gms Outdated Show resolved Hide resolved
modules/30_crop/rotation_apr22/presolve.gms Outdated Show resolved Hide resolved
@pvjeetze
Copy link
Contributor

pvjeetze commented Jun 3, 2022

Please also add some comparison figures to this PR for easier tracking of changes.

bodirsky and others added 7 commits June 5, 2022 08:02
Co-authored-by: Jan Dietrich <dietrich@pik-potsdam.de>
Co-authored-by: Jan Dietrich <dietrich@pik-potsdam.de>
Co-authored-by: Felicitas Beier <39262100+FelicitasBeier@users.noreply.github.com>
Co-authored-by: pvjeetze <50408549+pvjeetze@users.noreply.github.com>
…develop

# Conflicts:
#	config/default.cfg
#	modules/41_area_equipped_for_irrigation/static/presolve.gms
Copy link
Member Author

@bodirsky bodirsky left a comment

Choose a reason for hiding this comment

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

Can we maybe do the set-aside renaming in a separate pull request?
Its not directly connected to this one...

config/default.cfg Show resolved Hide resolved
config/default.cfg Outdated Show resolved Hide resolved
modules/30_crop/endo_apr21/declarations.gms Show resolved Hide resolved
modules/30_crop/endo_apr21/declarations.gms Show resolved Hide resolved
modules/30_crop/penalty_apr22/declarations.gms Outdated Show resolved Hide resolved
scripts/output/extra/disaggregation.R Show resolved Hide resolved
scripts/output/extra/disaggregation.R Outdated Show resolved Hide resolved
scripts/output/projects/fsdp_merge_report.R Outdated Show resolved Hide resolved
scripts/output/projects/fsdp_reporting_reg.R Outdated Show resolved Hide resolved
modules/30_crop/rotation_apr22/input/files Outdated Show resolved Hide resolved
…develop

# Conflicts:
#	CHANGELOG.md
#	config/default.cfg
additional = "additional_data_rev4.24.tgz",
cfg$input <- c(regional = "rev4.73_h12_magpie.tgz",
cellular = "rev4.73_h12_fd712c0b_cellularmagpie_c200_MRI-ESM2-0-ssp370_lpjml-8e6c5eb1.tgz",
validation = "rev4.73_h12_validation.tgz",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the new revision 4.73 include all RCPs too?

config/default.cfg Outdated Show resolved Hide resolved
@bodirsky bodirsky merged commit d7c837f into magpiemodel:develop Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Major Substantial modifications Medium risk
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants