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

nocc2020 switch added #277

Merged
merged 17 commits into from
May 19, 2021
Merged

nocc2020 switch added #277

merged 17 commits into from
May 19, 2021

Conversation

emolinab
Copy link
Contributor

@emolinab emolinab commented May 17, 2021

🐦 Purpose of this PR 🐦

  • A new option for the cc/nocc scenarios switch was added. Specifically, nocc2020 uses historical/variable values in parameters affected by climate change up to 2020. After 2020, values stay constant for the rest of the run's time horizon.

🔧 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
    Minor changes in scenario_config.csv, default.cgf, and several realizations.
  • 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 run based on the current version of the fork from which PR is made
  • 📉 Performance loss/gain from current default behavior 📈
    • Current develop branch default: 30 mins
    • This PR's default: 29.3 mins

image

  • 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).
  • [Not needed] Changes to magpie4 R library for post processing of model output (ideally backward compatible).
  • Self-review of my own code.
  • [Not needed] For high risk runs: validation of major model indicators - Land-use, emissions, food prices, Tau. %Delete this line in case it is not a high risk run%

⚠️ Additional notes or warnings ⚠️

NA

🚨 Checklist for RSE reviewer 🚨 -- @tscheypidi

  • 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 🚨 -- @abhimishr

  • 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

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.

Please check whether the solution via dollar condition as suggested would work too.

/ y2025, y2030, y2035, y2040,
y2045, y2050, y2055, y2060, y2065, y2070, y2075, y2080, y2085, y2090,
y2095, y2100, y2105, y2110, y2115, y2120, y2125, y2130, y2135, y2140,
y2145, y2150 /
;
Copy link
Member

Choose a reason for hiding this comment

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

I believe this set is not needed and a dollar condition could be used instead.

$if "%c14_yields_scenario%" == "nocc" f14_yields(t_all,j,kve,w) = f14_yields("y1995",j,kve,w);
$if "%c14_yields_scenario%" == "nocc2020" f14_yields(t_nocc2020,j,kve,w) = f14_yields("y2020",j,kve,w);
Copy link
Member

Choose a reason for hiding this comment

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

I believe a dollar condition should be a solution here setting the values as wanted. Haven't tested the suggestion below but something like that should be it:

Suggested change
$if "%c14_yields_scenario%" == "nocc2020" f14_yields(t_nocc2020,j,kve,w) = f14_yields("y2020",j,kve,w);
$if "%c14_yields_scenario%" == "nocc2020" f14_yields(t_all,j,kve,w)$(ord(t_all) > ord("y2020")) = f14_yields("y2020",j,kve,w);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect, I'll work in the dollar sign approach.

@tscheypidi
Copy link
Member

is it now low or medium risk? It can't be both as suggested by the labels

@emolinab emolinab added Minor Smaller modifications and removed Medium risk labels May 18, 2021
@emolinab emolinab requested a review from tscheypidi May 18, 2021 07:53
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 good!

Copy link
Member

@abhimishr abhimishr left a comment

Choose a reason for hiding this comment

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

Seems fine in general. I propose the following change to make it more consistent :

  1. Florian introduced a scalar sm_fix_ssp2 to keep our historical behavior consistent, maybe you can use the same switch here? By replacing your hard-coded 2020 instances with sm_fix_SSP2. It helps in two ways:
  • Everytime we update sm_fix_SSP2 the same is reflected in in you "holding constant after xx year" philosophy
  • Avoids hard-coding a number in $ checks
  1. Additionally, you can rename nocc2020 to histcc instead

@emolinab emolinab requested a review from abhimishr May 19, 2021 10:48
Copy link
Member

@abhimishr abhimishr left a comment

Choose a reason for hiding this comment

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

good to go 🚀

@emolinab emolinab merged commit 6e82e94 into magpiemodel:develop May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Low risk Low risk Minor Smaller modifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants