Bugfix in grasslands_apr22 pasture realization. #614

alexkoberle · 2023-11-17T18:47:38Z

🐦 Description of this PR 🐦

Briefly explain the purpose of this pull request

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

…pecific input files, required minor change in default.cfg.

…r sm_fix_SSP2 no longer used.

tscheypidi

looks good overall. Just some requests for modifications.

In addition to the ones mentioned it is also important to update the default.cfg to a newer data revision as soon as the new, compatible input data is available! Till then this PR should not be merged.

tscheypidi · 2023-11-17T19:11:12Z

CHANGELOG.md

@@ -12,6 +12,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - **14_yields_and_config** The new default is to not use yield calibration factors from a calibration run. The switch s14_use_yield_calib can optionally reenable the use of yield calibration factors.
 - **scripts** LUH2_disaggregation output script was modified. Specifically, flooded area was made compatible with the LUH definition, cropland and grazing land were added to the states.nc file, and specific naming/details (datatype,  zname, xname, and yname) were added when creating the .nc files.
 - **scripts** For the emulator scripts select a different bioenergy demand variable that excludes bioenergy sources other than second generation bioenergy crops. Set the minimal bioenergy demand to zero. Both avoid artificial clustering of data points and allow for better fits.
+- **default.cfg** Removed description of cfg$gms$c31_past_suit_scen since no longer needed due to changes in 31_past described below. Its function is now done by cfg$gms$c31_grassl_yld_scenario.


removal of switches fits better into the "removed" category

Moved to "removed" section of CHANGELOG.md

tscheypidi · 2023-11-17T19:14:09Z

modules/31_past/grasslands_apr22/input.gms

+i31_manpast_suit(t_all,j) = f31_pastr_suitability(t_all,j)
+* set values to 1995 if nocc scenario is used, or to sm_fix_cc after sm_fix_cc if nocc_hist is used
+$if "%c31_grassl_yld_scenario%" == "nocc" i31_manpast_suit(t_all,j) = f31_pastr_suitability("y1995",j);
+$if "%c31_grassl_yld_scenario%" == "nocc_hist" i31_manpast_suit(t_all,j)$(m_year(t_all) > sm_fix_cc) = i31_manpast_suit(t_all,j)$(m_year(t_all) = sm_fix_cc);


why has this been moved here? input.gms should only contain switch declarations and read statements for input files, not calculations like this one.

I followed @k4rst3ns recommendation to do this, following same logic as for f31_grassl_yld a few lines down (L41-42), and as done for the the yield module: https://github.com/magpiemodel/magpie/blob/master/modules/14_yields/managementcalib_aug19/input.gms

ok, in that case lets keep it here

…ovals.

alexkoberle · 2023-11-25T22:33:03Z

New input data created: rev4.95 ready to be used. Input vector updated in default.cfg. No new calibration file was generated in RSE folder so it was not changed from rev4.94

tscheypidi

Looks good

k4rst3ns · 2023-11-27T10:00:32Z

New input data created: rev4.95 ready to be used. Input vector updated in default.cfg. No new calibration file was generated in RSE folder so it was not changed from rev4.94

If you change the input revision, you also have to record the changes in the preprocessing, leading to a new input data revision. Please have a look here: https://gitlab.pik-potsdam.de/landuse/preprocessing-magpie/-/blob/main/CHANGELOG.md?ref_type=heads

pvjeetze

Looks good to me! (But consider comment by @k4rst3ns)

alexkoberle · 2023-11-27T14:09:07Z

New input data created: rev4.95 ready to be used. Input vector updated in default.cfg. No new calibration file was generated in RSE folder so it was not changed from rev4.94

If you change the input revision, you also have to record the changes in the preprocessing, leading to a new input data revision. Please have a look here: https://gitlab.pik-potsdam.de/landuse/preprocessing-magpie/-/blob/main/CHANGELOG.md?ref_type=heads

Done. Just pushed to pre-processing repo.

Alexandre Koberle added 2 commits November 17, 2023 16:05

Adjusted how f31_pastr_suitability is read in to align with ssp-rcp s…

e97eabd

…pecific input files, required minor change in default.cfg.

Adjusting 31_past to work with grasslands_apr22 realization. Paramete…

5085935

…r sm_fix_SSP2 no longer used.

alexkoberle added the bugfix label Nov 17, 2023

alexkoberle requested review from tscheypidi and pvjeetze November 17, 2023 18:47

tscheypidi requested changes Nov 17, 2023

View reviewed changes

Alexandre Koberle added 2 commits November 17, 2023 21:21

In CHANGELOG, moved switch removal in default.cfg from changes to rem…

a52044b

…ovals.

Bugfix to 31_past module grasslands_apr22 realization.

8f829ad

tscheypidi approved these changes Nov 25, 2023

View reviewed changes

alexkoberle requested a review from FelicitasBeier November 27, 2023 08:33

pvjeetze approved these changes Nov 27, 2023

View reviewed changes

alexkoberle merged commit 96c23d4 into magpiemodel:develop Nov 27, 2023
1 check passed

DavidhoPIK mentioned this pull request Nov 29, 2023

Inconsistency of merge #614 with scenario config #616

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix in grasslands_apr22 pasture realization. #614

Bugfix in grasslands_apr22 pasture realization. #614

alexkoberle commented Nov 17, 2023 •

edited by tscheypidi

Loading

tscheypidi left a comment

tscheypidi Nov 17, 2023

alexkoberle Nov 25, 2023

tscheypidi Nov 17, 2023

alexkoberle Nov 17, 2023

tscheypidi Nov 20, 2023

alexkoberle commented Nov 25, 2023

tscheypidi left a comment

k4rst3ns commented Nov 27, 2023

pvjeetze left a comment •

edited

Loading

alexkoberle commented Nov 27, 2023

Bugfix in grasslands_apr22 pasture realization. #614

Bugfix in grasslands_apr22 pasture realization. #614

Conversation

alexkoberle commented Nov 17, 2023 • edited by tscheypidi Loading

🐦 Description of this PR 🐦

🔧 Checklist for PR creator 🔧

📉 Performance changes 📈

🚨 Checklist for reviewer 🚨

tscheypidi left a comment

Choose a reason for hiding this comment

tscheypidi Nov 17, 2023

Choose a reason for hiding this comment

alexkoberle Nov 25, 2023

Choose a reason for hiding this comment

tscheypidi Nov 17, 2023

Choose a reason for hiding this comment

alexkoberle Nov 17, 2023

Choose a reason for hiding this comment

tscheypidi Nov 20, 2023

Choose a reason for hiding this comment

alexkoberle commented Nov 25, 2023

tscheypidi left a comment

Choose a reason for hiding this comment

k4rst3ns commented Nov 27, 2023

pvjeetze left a comment • edited Loading

Choose a reason for hiding this comment

alexkoberle commented Nov 27, 2023

alexkoberle commented Nov 17, 2023 •

edited by tscheypidi

Loading

pvjeetze left a comment •

edited

Loading