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

Improvment of NPI / NDC implementation + update of default settings + smaller bugfixes #768

Merged
merged 53 commits into from
Feb 3, 2025

Conversation

flohump
Copy link
Contributor

@flohump flohump commented Jan 23, 2025

🐦 Description of this PR 🐦

Summary: This PR fixes issues with the NPI/NDC implementation, adds switches to the scenario_config.csv file needed for coupling with REMIND, and fixes smaller bugs.

Note: CHANGELOG.md will be updated after release of version 4.9.1 (to avoid merge conflicts)

changed

  • scenario_config.csv cfg$gms$s56_minimum_cprice no longer used for NCD
  • config.cfg default for cfg$gms$cropland changed from "simple_apr24" to "detail_apr24"
  • config.cfg default for cfg$gms$s29_treecover_max changed from "0.4" to "0.5"
  • config.cfg default for cfg$gms$s29_fallow_max changed from "0.4" to 0

fixed

  • 29_cropland identical results for historic period when using s29_treecover_bii_coeff 0 and 1 in scenarios.
  • 32_forestry added contraint q32_ndc_aff_limit to make sure that NPI/NDC re/afforestation does not happen at the cost of forests and other natural vegetation.
  • 35_natveg added interface vm_natforest_reduction
  • 56_ghg_policy bugfixes for regional GHG policy fader

added

  • scenario_config.csv added column NPI-revert
  • scenario_config.csv added columns AR-natveg and AR-plant for CO2 price re/afforestation and AgroForestry settings
  • config.cfg added selection of low and middle-income countries isoCountriesLowMiddleIncome
  • start script for ScenarioMIP MAgPIE standalone runs

removed

  • scenario_config.csv removed column SSP2-EU

Resources_Land_Cover_Cropland-126
Productivity_Landuse_Intensity_Indicator_Tau-108
Emissions_CO2_Land_Land_use_Change-117

🔧 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 : 27 mins
  • This PR's default : 28 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)

@flohump flohump marked this pull request as ready for review January 23, 2025 16:46
Copy link
Contributor

@mishkos mishkos left a comment

Choose a reason for hiding this comment

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

Great from my side. Just a request to expand on new var and eq description.

modules/32_forestry/dynamic_may24/declarations.gms Outdated Show resolved Hide resolved
modules/32_forestry/dynamic_may24/declarations.gms Outdated Show resolved Hide resolved
@flohump flohump requested a review from mishkos January 28, 2025 21:25
Copy link
Contributor

@mishkos mishkos left a comment

Choose a reason for hiding this comment

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

All good! Thx!

@pascal-sauer
Copy link
Contributor

Thanks @flohump ! Looks all good to me now, will approve as soon as the release is done

CHANGELOG.md Outdated
- `29_cropland` identical results for historic period when using `s29_treecover_bii_coeff` 0 and 1 in scenarios.
- `32_forestry` added contraint `q32_ndc_aff_limit` to make sure that NPI/NDC re/afforestation does not happen at the cost of forests and other natural vegetation.
- `35_natveg` added interface `vm_natforest_reduction`
- `56_ghg_policy` bugfixes for regional GHG policy fader
Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent with the rest of the changelog I suggest to prefix everything with a category in bold (e.g. **scripts**)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint. done.

@flohump flohump merged commit 7c76926 into magpiemodel:develop Feb 3, 2025
1 check passed
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