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

dynamic age-class subset for plantation establishment depending on time step length #207

Merged
merged 15 commits into from
Jul 29, 2020

Conversation

flohump
Copy link
Contributor

@flohump flohump commented Jul 27, 2020

Please fill following information wherever applicable.

Purpose of this PR

Problem: Time steps with a length of more than 5 years cause gaps in the age-class structure of timber plantations.
So far, we manipulatd the level of v32_land in postsolve such that additions to plantations are distrubted over several age-classes (e.g. ac0 and ac5 for a 10 year time step). However, this is bad practice because it causes inconsistencies with carbon stocks, and problems with the carbonstock function in the magpie4 R library.
Now, there is a dynamic subset "ac_est", which depedends on the time step length (e.g. ac0 and ac5 for a 10 year time step). Together with the new constraint "q32_forestry_est" this allows to distribute the establishment over several age-classes within the optimization. This makes sure that carbon stocks (and all other variables) are consistent. Manipulations in postsolve are no longer needed. The carbonstock function in the magpie4 R library has been updated accordingly.
The counterpart of ac_est is ac_sub, which is also dynamic. This required a change in the declaration of some variables and paramters. Several unnecessary sets and parameters have been removed.

Performance loss/gain from current default behavior

No difference in run time.

Unknown-37

Type of change

  • [x ] Bug fix (Change which fixes an issue).
  • New feature (Change which adds functionality).
  • Minor change (Change which does not modify core model code i.e., in /modules).
  • Major change (fix or feature that would change current model behavior).

How Has This Been Tested?

forestry_test start script.
One run before the bugfix. One run after the bugfix.

Default configuration additions/changes:

  • Realizations:
  • Scalars/Constants:
  • Model interfaces added:
  • Other changes:

Checklist:

  • Self-review of my own code.
  • [not needed for bugfix] Added changes to CHANGELOG.md
  • [ x] No hard coded numbers and cluster/country/region names.
  • [to my best knowledge: Yes ] The code doesn't contain declared but unused parameters or variables.
  • [ x] In-code comments added including documentation comments.
  • [ x] Compiled and checked resulting documentation with goxygen::goxygen() for the new/updated parts.
  • Changes to magpie4 R library for post processing of model output (ideally backward compatible).

Special comments/warnings

  • Notes

Resources_Land_Cover_Forest_Managed_Forest_Plantations.pdf
Emissions_CO2_Land_Land_use_Change-10.pdf

@flohump flohump added bug Something isn't working Minor Smaller modifications Low risk Low risk labels Jul 27, 2020
@flohump flohump requested a review from abhimishr July 27, 2020 08:01
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.

In principle it looks good to go but I added some requests.

Also can you edit the PR using the draft template from https://raw.githubusercontent.com/magpiemodel/magpie/7c481557d9ecf66582ab92c7e875259cad33228c/.github/pull_request_template.md

core/calculations.gms Outdated Show resolved Hide resolved
modules/32_forestry/dynamic_may20/presolve.gms Outdated Show resolved Hide resolved
@flohump flohump requested a review from abhimishr July 27, 2020 21:11
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.

Thank you for addressing the small changes. For me looks good to go!

@abhimishr
Copy link
Member

@tscheypidi we still need RSE side approval for bugfix PRs?

@flohump flohump merged commit a1c5bf3 into magpiemodel:develop Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Low risk Low risk Minor Smaller modifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants