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

Grassland management implementation #379

Merged
merged 63 commits into from
Apr 6, 2022

Conversation

mppalves
Copy link
Contributor

@mppalves mppalves commented Mar 2, 2022

🐦 Purpose of this PR 🐦

This pull request implements a new realization of module 31_past that allows the separation of grass biomass production into areas of managed pastures and rangelands. It also updates module 13_tc adding a new set that distinguishes tau that is applied to managed pastures vs tau applied to crops.

🔧 Checklist for PR creator 🔧

  • Low risk : Simple bugfixes (missing files, updated documentation, typos) or Start/output scripts
  • Medium risk : New realization / Changes to existing realization / Other changes which don't modify default.cfg
  • High risk : New input files (if cfg$input is changed in default.cfg) / Modification to core model (eg. changes in equations, calculations, introduction of new sets etc.) / Other changes in default.cfg
  • Providing additional information based on PR label
  • Low risk : No new model run needed.
  • Medium risk : Default run based on the current version of the fork from which PR is made
  • High risk
    • Default run from the current develop branch
      default_dev_mar22
    • Default run based on the current version of the fork from which PR is made
      default_new_mar22 - New default
      IPSL-CM6A-LR-SSP2-set18 - New Realization ON
  • 📉 Performance loss/gain from current default behavior 📈

    • Current develop branch default : 30.7 mins
    • This PR's default : 32.0 mins
  • Added changes to CHANGELOG.md

  • Compilation check (model starts without compilation errors - use gams main.gms action=c in model folder for testing).

  • No hard coded numbers and cluster/country/region names.

  • The new code doesn't contain declared but unused parameters or variables.

  • Where relevant, In-code comments added including documentation comments.

  • Made sure that documentation created with goxygen is okay (use goxygen::goxygen() for testing).

  • Changes to magpie4 R library for post processing of model output (ideally backward compatible).

  • Self-review of my own code.

  • For high risk runs: validation of major model indicators - Land-use, emissions, food prices, Tau.

=======================================================================

**Below find the results comparing the current default and the new implementation: **

The blue line is the current develop (old TC module and old pasture module)
The red line is the new proposed default (new TC module and old pasture module)
The green line is the new implementation (new TC and new pasture modules)

Resources_Land_Cover_Pastures_and_Rangelands
Resources_Land_Cover_Cropland
Productivity_Landuse_Intensity_Indicator_Tau
Resources_Land_Cover_Managed_Pastures
Resources_Land_Cover_Rangelands

⚠️ 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

@mppalves mppalves added enhancement New feature or request Major Substantial modifications High risk Higher risk labels Mar 2, 2022
@flohump flohump requested a review from weindl March 2, 2022 16:21
@mppalves mppalves requested review from tscheypidi and weindl and removed request for weindl March 21, 2022 13:07
@mppalves mppalves changed the title Develop clean 32 Grassland implementation Apr 1, 2022
@mppalves mppalves changed the title Grassland implementation Grassland management implementation Apr 1, 2022
@mppalves mppalves requested review from weindl and tscheypidi and removed request for flohump, tscheypidi and weindl April 1, 2022 13:05
Copy link
Member

@tscheypidi tscheypidi left a comment

Choose a reason for hiding this comment

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

looks quite good now. Just one additional suggestion for further simplification and two questions concerning some mz-files which are now being copied to the modules

modules/31_past/input/files Outdated Show resolved Hide resolved
modules/40_transport/input/files Outdated Show resolved Hide resolved
@mppalves mppalves requested a review from tscheypidi April 4, 2022 13:04
Copy link
Contributor

@weindl weindl 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!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
config/default.cfg Show resolved Hide resolved
@mppalves mppalves merged commit 46a29da into magpiemodel:develop Apr 6, 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 High risk Higher risk Major Substantial modifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants