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.

Sign up for GitHub

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

Jump to bottom

Add GDP share constraint for tech cost #391

Merged
merged 5 commits into from
Apr 28, 2022
Merged

Add GDP share constraint for tech cost #391

merged 5 commits into from
Apr 28, 2022

Conversation

jansteinhauser
Copy link
Contributor

@jansteinhauser jansteinhauser commented Apr 26, 2022 •
edited by flohump
Loading

Change tech cost upper bound to a config-set GDP share

🐦 Purpose of this PR 🐦

  • This PR changes the tech cost upper bound in the endo TC realization to a permille share ofthe regional GDP (PPP). As the current default is purely technical, this is a necessary constraint for price-driven runs that may push TC and TC cost to extreme heights.

🔧 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 run based on the current version of the fork from which PR is made

Costs_TC

Productivity_Landuse_Intensity_Indicator_Tau_cropland

  • 📉 Performance loss/gain from current default behavior 📈

    • Current develop branch default : 33 mins
    • This PR's default : 37 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. %Delete this line in case it is not a high risk run%

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

@jansteinhauser
Add GDP share constraint for tech cost
131ee47 
Expand tech cost upper bound to enable switching
between fixed bound and a config-set GDP share
@jansteinhauser
Update CHANGELOG.md
6de73a7
@jansteinhauser
Remove TC Cost switch and old default
af651d7 
Remove former default upper bound and related switch
as 0.002 gdp ppp share bound and level guidance via upper
bound enable and produce similar results, making switching an
unnecessary option.
@jansteinhauser jansteinhauser marked this pull request as ready for review April 27, 2022 15:03
@jansteinhauser
Copy link
Contributor Author

jansteinhauser commented Apr 27, 2022 •
edited
Loading

It seems I can neither request reviews nor label the pull request (if I had to guess, it's because I am a first-time contributor), so if someone would be so kind to label this PR as medium risk (as it's changing a realization) and enhancement [Edit: actually, minor, seems like a better choice. Missed that earlier], that would be grand!
Also @tscheypidi @flohump would you kindly review this PR?
Thank you!

@tscheypidi tscheypidi added Minor Smaller modifications Medium risk labels Apr 27, 2022
tscheypidi
tscheypidi requested changes Apr 27, 2022
View reviewed changes
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.

Thanks for your first PR! (hooray)

Just 2 minor requests from my side.

CHANGELOG.md Outdated
@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]

### changed
- **config** added s13_tech_cost_gdp_share setting for tech cost upper bound as share of GDP PPP
Copy link
Member

Choose a reason for hiding this comment

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

The name of the scalar is a bit misleading. Would be good to make more clear that we are talking about an upper bound here. Perhaps you can write something like s13_max_gdp_shr instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

modules/13_tc/endo_jan22/presolve.gms Outdated
vm_tech_cost.up(i) =
sum((i_to_iso(i,iso),ct), im_gdp_pc_ppp_iso(ct,iso) * im_pop_iso(ct,iso)) * s13_tech_cost_gdp_share;

vm_tech_cost.L(i) = vm_tech_cost.up(i);
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here explaining why the level is being used (and highlight that it is in fact necessary to help the solver finding a proper solution).

In addition: is that normally a capital "L" for level? I always thought it is a small "l".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added and changed both.
The capital L was just me being inconsistent.

tscheypidi
tscheypidi approved these changes Apr 28, 2022
View reviewed changes
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

flohump
flohump approved these changes Apr 28, 2022
View reviewed changes
Copy link
Contributor

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

@tscheypidi tscheypidi merged commit 2f20609 into magpiemodel:develop Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tscheypidi tscheypidi tscheypidi approved these changes

@flohump flohump flohump approved these changes

Assignees
No one assigned
Labels
Medium risk Minor Smaller modifications
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jansteinhauser @tscheypidi @flohump