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

Fixes in rc after test runs for forestry and default settings #214

Merged
merged 105 commits into from
Sep 15, 2020

Conversation

abhimishr
Copy link
Member

@abhimishr abhimishr commented Aug 21, 2020

Purpose of this PR

Purpose of this PR is to apply some fixes for forestry implementation and to also update the module embedding issues in module 38 Factor Costs

Performance loss/gain from current default behavior

magpie default baseline: 0.58
magpie default policy : 0.85
magpie forestry baseline: 0.9 (performance loss = 60%)

Untitled

Type of change

  • 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 behaviour).

How Has This Been Tested?

  • Runs using starting script which tests current defaults (using test_runs.R).
  • Runs using starting script which successfully runs the same scenarios as in test_runs.R but with the changes from PR.
    Note - No CO2 price runs
  • (If applicable) Runs using different scenarios/mappings which are not the default (12 regions/c200 clusters).

Additions or Changes to default configuration (default.cfg):

Additions are the introduction of new model components in default config

Changes are deletion or updates to the existing model components in default config

  • Realizations:
  • Scenario switches: See changelog
  • Scalars/Constants: See changelog
  • Model interfaces:
  • Others:

Checklist:

  • Self-review of my own code.
  • Added changes to CHANGELOG.md
  • No hard coded numbers and cluster/country/region names.
  • The code doesn't contain declared but unused parameters or variables.
  • In-code comments added including documentation comments.
  • 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

None. This is a fix in rc branch which will be made master and remerged with develop so the changes are made directly to the release candidate.

@abhimishr abhimishr added bug Something isn't working enhancement New feature or request Suggestion Minor Smaller modifications Low risk Low risk labels Aug 21, 2020
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.

The changes look fine to me. Please add the details in default.cfg.
Please check if your switch works as expected (in the results) before merging.

# If the share of overall woody biomass demand coming from plantation be kept
# constant at 1995 levels or if this should be increasing with time
# * "increasing" : Increasing share
# * "constant" : Constant share
Copy link
Contributor

Choose a reason for hiding this comment

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

please add these details:
constant means constant at 33%
increasing means linear increase from 33% to 66% until 2100, based on literature

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

CHANGELOG.md Outdated Show resolved Hide resolved
@abhimishr abhimishr requested a review from flohump August 21, 2020 23:34
@abhimishr
Copy link
Member Author

Please check if your switch works as expected (in the results) before merging.

The switch works as expected in controlling the share of production coming from plantations

@@ -253,7 +253,8 @@ sets
***Forestry**
ac Age classes / ac0,ac5,ac10,ac15,ac20,ac25,ac30,ac35,ac40,ac45,ac50,
ac55,ac60,ac65,ac70,ac75,ac80,ac85,ac90,ac95,ac100,
ac105,ac110,ac115,ac120,ac125,ac130,ac135,ac140,ac145,acx /
ac105,ac110,ac115,ac120,ac125,ac130,ac135,ac140,ac145,
ac150,ac155,acx /
Copy link
Member Author

Choose a reason for hiding this comment

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

Added new age-classes for making sure than carbonstock reporting is not buggy in case MAgPIE is run till 2150

@@ -16,5 +16,5 @@ kpr(kall) Products that can be processed
knpr(kall) Plant-based products that cannot be processed
/alcohol,distillers_grain,ethanol,fibres,fish,livst_chick,livst_egg,livst_milk,
livst_pig,livst_rum,oils,oilcakes,pasture,puls_pro,res_cereals,res_fibrous,
res_nonfibrous,scp/
res_nonfibrous,scp,wood,woodfuel/
Copy link
Member Author

Choose a reason for hiding this comment

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

Added for clarity - We don't model processing of initial wood products

@@ -16,7 +16,7 @@ kpr(kall) Products that can be processed
knpr(kall) Products that cannot be processed
/alcohol,distillers_grain,ethanol,fibres,fish,livst_chick,livst_egg,livst_milk,
livst_pig,livst_rum,oilcakes,pasture,puls_pro,res_cereals,res_fibrous,
res_nonfibrous,scp/
res_nonfibrous,scp,wood,woodfuel/
Copy link
Member Author

Choose a reason for hiding this comment

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

Added for clarity - We don't model processing of initial wood products

@@ -5,7 +5,7 @@
*** | MAgPIE License Exception, version 1.0 (see LICENSE file).
*** | Contact: magpie@pik-potsdam.de

$setglobal c21_trade_liberalization l908080r807070
$setglobal c21_trade_liberalization l909090r808080
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the value from default config. Not sure why this was set to l908080r807070 by default

CHANGELOG.md Outdated
- **57_maccs** Added MACCs from Harmsen PBL 2019
- **15_food** Added the option to include calories from alcohol consumption in healthy and sustainable EAT-Lancet diets.
- **scripts** added start script for making timber production runs (forestry.R)
- **switch** Allowing for constant or increasing area of timber plantations area after 2020, allowing to fix a static portion of overall timber production coming from plantations, added for allowing a using a forward looking or myopic timber demand for plantation establishment
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the second part of the sentence (starting with "allowing"). The reasoning behind a change also does not necessarily need to be mentioned here, so you could simply skip that part. In addition: You might rather refer to the module the switch has been introduced to rather than calling that entry "switch".
And it would be helpful to mention the name of the switch

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

CHANGELOG.md Outdated
- **57_maccs** Added MACCs from Harmsen PBL 2019
- **15_food** Added the option to include calories from alcohol consumption in healthy and sustainable EAT-Lancet diets.
- **scripts** added start script for making timber production runs (forestry.R)
- **switch** Allowing for constant or increasing area of timber plantations area after 2020, allowing to fix a static portion of overall timber production coming from plantations, added for allowing a using a forward looking or myopic timber demand for plantation establishment
- **scalars** Global interest rate for timber plantation's rotation length calculation, additional investment needed to setup timber plantations in unproductive cells.
Copy link
Member

Choose a reason for hiding this comment

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

please mention the names of the introduced scalars and provide the name of the affected modules at the beginning rather than calling it "scalars"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Affected modules are mentioned trailing the mentioning of scalars/switches

@@ -456,6 +456,22 @@ cfg$gms$c32_tcre_ctrl <- "ann_TCREmean"
# * 0=off
cfg$gms$s32_initial_distribution <- 0 # def = 0

# Switch to fix timber plantations area after sm_fix_SSP2 value
# This makes sure that cellular timber plantations are not changed after 2020.
cfg$gms$s32_fix_plant <- 0 # def = 0
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how that switch works. What means 0? No fixing? What would be alternative values to put here? And what would they mean? Does it have a unit? How is that all related to 2020? What happens before 2020? Does it mean that this number only affects the behavior after 2020?

Copy link
Member Author

Choose a reason for hiding this comment

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

The number 0 indeed means plantations are not fixed after 2020. The plantations are also not fixed before 2020 irrespective of the settin in s32_fix_plant. I added available settings for this in description and default.cfg. It does not have a unit, just a binary switch. It breaks model behavior if the values are other than 0 or 1 so for making sure that the model still works if somebody sets it to other than 0 or 1 I can add an additional check.


# Switch to use a constant share of overall timber production from plantations
# This also means that the model only sees a constant production share from future
cfg$gms$s32_plant_share <- 0.25 # def = 0.25
Copy link
Member

Choose a reason for hiding this comment

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

Again the description is not clear to me. The beginning of the sentence suggests that via the switch I am able to switch betweena constant share and another scenario, but looking at the value my guess is that we are always talking about a constant share scenario and I just define the share here. Is that correct? If so, please describe it that way in the comment. If my interpretation is correct you could for instance write: "Share of overall timber production from plantations which is hold constant over time."

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. This is better. I changed it accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added additional check to make sure a negative value or a value greater than 1 is not set by user

cfg$gms$s32_plant_share <- 0.25 # def = 0.25

# Switch to activate regional or global interest rate for rotation length calculations
cfg$gms$c32_interest_rate <- "regional" # def = regional
Copy link
Member

Choose a reason for hiding this comment

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

Here I guess you have some predefined options what the switch could be set to. Please mention all options here! (Have a look at other switches as an example how to do so)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

cfg$gms$s32_forestry_int_rate <- 0.05 # def = 0.05

# Additional investment cost in plantations for using unproductive cells
cfg$gms$s32_investment_cost <- 200 # def = 1000
Copy link
Member

Choose a reason for hiding this comment

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

values should be set to the default value in default.cfg

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -644,6 +660,9 @@ cfg$gms$carbon <- "normal_dec17" # def = normal_dec17
# * nocc (no climate change)
cfg$gms$c52_carbon_scenario <- "nocc" # def = "nocc"

# * Minimum threshold of carbon density in timber plantations
cfg$gms$s52_plantation_threshold <- 8 # def = 5

Copy link
Member

Choose a reason for hiding this comment

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

What is the unit here? Please set value to its default value!

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the unit and updated the default values


q73_cost_timber(i2)..
vm_cost_timber(i2)
=e=
v73_cost_hvarea(i2)
+ sum((cell(i2,j2),kforestry), v73_prod_heaven_timber(j2,kforestry)) * s73_free_prod_cost
+ sum((cell(i2,j2),ac,kforestry), v73_prod_forestry(j2,ac,kforestry) * s73_timber_harvest_cost * 0)
Copy link
Member

Choose a reason for hiding this comment

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

multiplication by 0? Is that intended? If so, delete the whole line instead of multiplying it with 0!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

+ sum(ac_sub, v73_hvarea_other(j2, ac_sub))
+ vm_hvarea_primforest(j2)) * (s73_timber_harvest_cost * s73_cost_multiplier))
sum((ct,cell(i2,j2),ac_sub), vm_hvarea_forestry(j2,ac_sub) * s73_timber_harvest_cost)
+ sum((ct,cell(i2,j2),ac_sub), vm_hvarea_secdforest(j2,ac_sub) * s73_timber_harvest_cost * s73_cost_multiplier * 0.75)
Copy link
Member

Choose a reason for hiding this comment

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

what does the 0.75 mean here? Where does it come from. Better: Use a scalar containing that value and explain in the description of the scalar what it means and what unit it has.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Converted to a parameter which is filled in during preloop for secondary forest, primary forest and other land,

Copy link
Member Author

Choose a reason for hiding this comment

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

And this is unitless, just a multiplicative factor

+ vm_hvarea_primforest(j2)) * (s73_timber_harvest_cost * s73_cost_multiplier))
sum((ct,cell(i2,j2),ac_sub), vm_hvarea_forestry(j2,ac_sub) * s73_timber_harvest_cost)
+ sum((ct,cell(i2,j2),ac_sub), vm_hvarea_secdforest(j2,ac_sub) * s73_timber_harvest_cost * s73_cost_multiplier * 0.75)
+ sum((ct,cell(i2,j2),ac_sub), vm_hvarea_other(j2, ac_sub) * s73_timber_harvest_cost * s73_cost_multiplier * 0.75)
Copy link
Member

Choose a reason for hiding this comment

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

same as comment before

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Same as above.

@tscheypidi tscheypidi merged commit ed6cce0 into magpiemodel:rc Sep 15, 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 enhancement New feature or request Low risk Low risk Minor Smaller modifications Suggestion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants