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

Ignore historic TAU patterns + land conversion costs forestry + Bugfix BII + Bugfix NPI/NDC afforestation #276

Merged
merged 33 commits into from
May 26, 2021

Conversation

flohump
Copy link
Contributor

@flohump flohump commented May 10, 2021

🐦 Purpose of this PR 🐦

  • Ignore historic TAU patterns for improved near-term trends of cropland and CO2 emission trends
  • lower land conversion costs for forestry (1000 $/ha). High land conversion costs (8000 $/ha) are only needed for cropland and pasture to maintain a balance between expansion and intensification. High land conversion costs for forestry land prohibit afforestation. Lower land conversion costs for forestry increase afforestation in Policy runs but do not change timber plantation establishment.
  • Bugfix BII: ac0 included in pricing of biodiversity loss, BII coefficients for CO2 price driven afforestation
  • Bugfix NPI/NDC afforestation: the previous formulation caused infeasibilies in some cases.
  • Bugfix: growth curve for CO2 price driven afforestation was wrong in case of activated forestry ("ForestryEndo")

Results: https://1drv.ms/p/s!AlN2Seu8OCz8hJRPNljEb1sVbRPBeA
Additional results can be added on request.

🔧 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

Start script for test runs: TAUhistfree.R

  • 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 : 0.48 h
    • This PR's default : 0.44 h
  • 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.

  • validation of major model indicators - Land-use, emissions, Tau -> see link to shared results pptx above

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

@flohump flohump added bug Something isn't working enhancement New feature or request Minor Smaller modifications Low risk Low risk labels May 10, 2021
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 rename/reformulate your TC modifications as the current name and description is a bit misleading
  • please also add the new switch to the config file
  • if you plan to change the default behavior and abandon the historic tau development this probably requires more discussions and would make it a "high risk" PR

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -6,6 +6,10 @@
*** | Contact: magpie@pik-potsdam.de


scalars
s13_tau_hist_lower_bound logical switch for setting a lower bound on TAU in historic time steps (0=off 1=on) / 0 /
Copy link
Member

Choose a reason for hiding this comment

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

you should call it and describe it different, e.g. s13_ignore_tau_historical. As this is a on/off switch you also might want to think about using a switch rather than a scalar for it

Copy link
Member

Choose a reason for hiding this comment

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

In addition, this switch should also show up in the config then. Do you plan to change the default here (not taking historical values in to account anymore)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to s13_ignore_tau_historical
Do we really need this switch in the config? It can be changed in a start script, if needed by expericend users.

Copy link
Member

Choose a reason for hiding this comment

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

my understanding was that you introduced that scalar to be able to change the setting...if that was the intention than it should be part of the config.
If your intention was just to ignore historic tau values in all future simulations the historic values should be removed from the realization alltogether...however, in the latter case this is a significant modification and should be discussed as for all other values we fix the model to historic values. Excluding tau values from that rule would mean a paradigm shift

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree here. First, we have other examples of switches which exist in an input.gms file but are not part of the config file. My understanding is that this serves the gaol of not overloading the default.cfg file with too many switches. However, I'm fine with adding s13_ignore_tau_historical to the default.cfg.
Second, and this is more important, we don't fix or impose a lower bound on cropland in the historic period. Having a lower bound only for TAU is somewhat unbalanced from my perspective, and partly explains why our near-term dynamics of cropland and CO2 emissions slightly mismatch with observed data. Setting a lower bound or fixing a variable for historic time steps is rather an exception so far. We fix many parameter values to make sure that scenarios don't diverge before 2020.
I now created additional runs based on the current develop and the branch to be merged to facilitate a structured, efficient and transparent discussion.
Results can be found here: https://1drv.ms/p/s!AlN2Seu8OCz8hJRPNljEb1sVbRPBeA

sum(ac_mature, v32_land(j2,"aff",ac_mature)) * fm_luh2_side_layers(j2,potnatveg) *
(fm_bii_coeff("secd_mature",potnatveg)*(1-s32_aff_plantation) + fm_bii_coeff("timber",potnatveg)*(s32_aff_plantation))
+ sum(ac_young, v32_land(j2,"aff",ac_young)) * fm_luh2_side_layers(j2,potnatveg) *
(fm_bii_coeff("secd_young",potnatveg)*(1-s32_aff_plantation) + fm_bii_coeff("timber",potnatveg)*(s32_aff_plantation));
Copy link
Member

Choose a reason for hiding this comment

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

This is quite some repetitive writing. Isn't there a better formulation possible, e.g. via a mapping set rather than writing 2 times the same for ac_mature and ac_young?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I revised these equations. Please check.

(fm_bii_coeff("secd_mature",potnatveg)*(1-s32_timber_plantation) + fm_bii_coeff("timber",potnatveg)*(s32_timber_plantation))
+ sum(ac_young, v32_land(j2,"plant",ac_young)) * fm_luh2_side_layers(j2,potnatveg) *
(fm_bii_coeff("secd_young",potnatveg)*(1-s32_timber_plantation) + fm_bii_coeff("timber",potnatveg)*(s32_timber_plantation));

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 above....are the repeated terms the same as above? Perhaps calculate them first in a separate equations and use them afterwards in these two equations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I revised these equations. Please check.

s39_cost_establish_crop Cost for establishing new land use (USD05MER per hectare) /8000/
s39_cost_establish_past Cost for establishing new land use (USD05MER per hectare) /8000/
s39_cost_establish_forestry Cost for establishing new land use (USD05MER per hectare) /1000/
s39_cost_clearing Clearing costs linked to removed biomass (USD05MER per ton C) /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 believe this change in settings requires the PR to be tagged as "medium risk" as it will have probably some visible effects on the dynamics

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. Happy to discuss these changes in the next group meeting.

@flohump flohump added High risk Higher risk and removed Low risk Low risk labels May 10, 2021
Copy link
Contributor

@pvjeetze pvjeetze left a comment

Choose a reason for hiding this comment

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

Overall fine, thanks for fixing the biodiversity module!

/ crop_ann, crop_per, manpast, rangeland, urban, primary, secd_mature, secd_young, timber /

bii_secd(bii_class44) bii coefficent land cover classes secondary vegetation
Copy link
Contributor

Choose a reason for hiding this comment

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

First impression is that it becomes a little harder to read initially, with this additional set. But this may be very personal and the code may get pretty long otherwise. Just to highlight this. Maybe clearer (but longer names) could help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could rename "bii_secd" to "bii_class44_secd". But this would imply renaming "ac_to_bii_secd(ac,bii_secd)" to "ac_to_ bii_class44_secd(ac, bii_class44_secd)", which would also increase the length of the code in equations.gms. Maybe we just call it "bii_ac"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, I think. For the second one maybe the other way around ac_bii?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you suggest to rename "bii_secd" to "ac_bii"?

Copy link
Contributor

@pvjeetze pvjeetze May 11, 2021

Choose a reason for hiding this comment

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

no excuse me. bii_secd -> bii_class44_secd and ac_to_bii_secd -> ac_bii/ac_bii_secd

@flohump flohump changed the title Remove lower bound on TC in historic time steps + lower land conversion costs for forestry + Bugfix BII Ignore historic TAU patterns + lower land conversion costs for forestry + Bugfix BII May 11, 2021
@flohump flohump changed the title Ignore historic TAU patterns + lower land conversion costs for forestry + Bugfix BII Ignore historic TAU patterns + land conversion costs forestry + Bugfix BII + Bugfix NPI/NDC afforestation May 13, 2021
@flohump flohump requested a review from mishkos May 13, 2021 08:09
Copy link
Contributor

@pvjeetze pvjeetze left a comment

Choose a reason for hiding this comment

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

Thanks Florian for the updates! I've looked at them once more and all is fine.

Copy link
Contributor

@weindl weindl left a comment

Choose a reason for hiding this comment

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

Technically, the modifications look good. The comments on NPI/NDC afforestation are suggestions, and do not preclude a merge of the PR.

** This allows to use plantation growth curves for CO2 price driven afforestation.
p32_c_density_ac_fast_forestry(t_all,j,ac) = pm_carbon_density_ac_forestry(t_all,j,ac,"vegc");

** Update c-density for timber plantations based on calibration factor to match FAO growing stocks
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure:
While timber plantation uses the FAO-calibrated carbon density (pm_carbon_density_ac_forestry(t_all,j,ac,"vegc") ), carbon-price induced afforestation uses the uncalibrated carbon density (p32_c_density_ac_fast_forestry(t_all,j,ac) ), right?

This allows afforestation to follow the growth curves towards LPJmL natural vegetation carbon density, without being inconsistent with the LPJmL estimates at the mature level.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked in modules/32_forestry/dynamic_feb21/presolve.gms the application of the carbon density parameters. All looks fine.

Copy link
Contributor Author

@flohump flohump May 14, 2021

Choose a reason for hiding this comment

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

Just to be sure:
While timber plantation uses the FAO-calibrated carbon density (pm_carbon_density_ac_forestry(t_all,j,ac,"vegc") ), carbon-price induced afforestation uses the uncalibrated carbon density (p32_c_density_ac_fast_forestry(t_all,j,ac) ), right?

Yes, I confirm. That's how it is intended. I discussed this @abhimish

This allows afforestation to follow the growth curves towards LPJmL natural vegetation carbon density, without being inconsistent with the LPJmL estimates at the mature level.

elseif s32_aff_bii_coeff = 1,
p32_bii_coeff("aff",bii_class_secd,potnatveg) = fm_bii_coeff("timber",potnatveg)
);
p32_bii_coeff("ndc",bii_class_secd,potnatveg) = fm_bii_coeff(bii_class_secd,potnatveg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to also allow for the application of the switch "s32_aff_bii_coeff" onto NPI/NDC afforestation? Also here, afforestation policies could be implemented in different ways, either in a more plantation like manner with fast growing species or using only native species, with different implications for biodiversity and thus the choice of bii coefficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this could be added. We would need 2 more switches for that like s32_ndc_growth_curve and s32_ndc_bii_coeff to allow for different assumptions with respect to ndc and co2 price driven afforestation. @mishkos What do you think?Does it make sense to have these additional switches?

Copy link
Contributor

Choose a reason for hiding this comment

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

While it does make sense to differentiate what kind of forests are established under npi/ndc policies, the national documents don't specify this at all. Therefore I suggest for the time being to stick to the natveg growth curves for npi/ndc aff pol.

Copy link
Member

Choose a reason for hiding this comment

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

NPI/NDC afforestation still uses natveg growth. We only now provide a possibility where CO2 price induced afforestation can be selected to grow with a plantation growth curved. We have three forestry categories, plant which is timber production plantations, aff which are co2 priced afforestation and ndc which are prescribed afforestation according to NPI/NDC policy. My understanding was that prescribed NPI/NDC policy does not merge back into aff.
@flohump this is the correct way to put it right?


*' Timber plantations carbon densities:
p32_carbon_density_ac(t,j,"plant",ac,ag_pools) = pm_carbon_density_ac_forestry(t,j,ac,ag_pools);

*' NDC carbon densities are natveg carbon densities.
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment on bii coefficient in the NPI/NDC context above: maybe consider also here the possibility to select between plantation and natveg growth curves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This I discussed with @abhimishr . We agreed that it does not make sense to assume natveg growth curves for timber plantations.

Copy link
Contributor

Choose a reason for hiding this comment

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

as answered above, I suggest to keep natveg growth curves for the npi/ndc aff policy.

@abhimishr
Copy link
Member

Also seems fine to me. As we need to finish the checklist part, @pvjeetze or @weindl would you like to do it? if not I can also do it. Just indicate this here. MAgPIE side of review seems good to go and @tscheypidi will handle the RSE side of review.

@abhimishr
Copy link
Member

Also tagging @mishkos here. The cost validation taskforce indicated about timber production costs in non-rimber production runs for historical period. Would it be okay if I fix it in the post-processing side?

Copy link
Contributor

@mishkos mishkos left a comment

Choose a reason for hiding this comment

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

Approving the changes that are correcting the cases of NPI/NDC afforestation infeasibility

p32_aff_pol_timestep(t,j)$(ord(t)>1) = p32_aff_pol(t,j) - p32_aff_pol(t-1,j);
* Limit prescribed NPI/NDC afforestation in `p32_aff_pol_timestep` if not enough suitable area (`p32_aff_pot`) for afforestation is available
p32_aff_pol_timestep(t,j)$(p32_aff_pol_timestep(t,j) > p32_aff_pot(t,j)) = p32_aff_pot(t,j);
** END ndc **
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good to me. The change doesn't affect the way how npi/ndc aff policy is established in each cell. The code is simplified.

elseif s32_aff_bii_coeff = 1,
p32_bii_coeff("aff",bii_class_secd,potnatveg) = fm_bii_coeff("timber",potnatveg)
);
p32_bii_coeff("ndc",bii_class_secd,potnatveg) = fm_bii_coeff(bii_class_secd,potnatveg);
Copy link
Contributor

Choose a reason for hiding this comment

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

While it does make sense to differentiate what kind of forests are established under npi/ndc policies, the national documents don't specify this at all. Therefore I suggest for the time being to stick to the natveg growth curves for npi/ndc aff pol.


*' Timber plantations carbon densities:
p32_carbon_density_ac(t,j,"plant",ac,ag_pools) = pm_carbon_density_ac_forestry(t,j,ac,ag_pools);

*' NDC carbon densities are natveg carbon densities.
Copy link
Contributor

Choose a reason for hiding this comment

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

as answered above, I suggest to keep natveg growth curves for the npi/ndc aff policy.

@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### fixed
- **32_foresty** BII coefficients for CO2 price driven afforestation
Copy link
Contributor

Choose a reason for hiding this comment

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

Please briefly highlight new switch for choosing the BII coefficient

@abhimishr
Copy link
Member

@tscheypidi would you like to request more changes? I'll re-request your review here. MAgPIE side of review seems good to go.

@abhimishr abhimishr requested a review from tscheypidi May 21, 2021 08:10
@tscheypidi
Copy link
Member

@abhimishr If i remember correctly, we postponed in our last MAgPIE meeting the decision how to deal with the suggested modification of historic tau treatment and wanted to discuss it again in the next meeting. That is why I haven't given green light yet

@flohump
Copy link
Contributor Author

flohump commented May 21, 2021

@abhimishr If i remember correctly, we postponed in our last MAgPIE meeting the decision how to deal with the suggested modification of historic tau treatment and wanted to discuss it again in the next meeting. That is why I haven't given green light yet

In cooperation with @k4rst3ns I included additiontal results based on #273 in the results document.
https://1drv.ms/p/s!AlN2Seu8OCz8hJRPNljEb1sVbRPBeA

Ignoring historic TAU patterns has mostly effects on cropland dynamics and CO2 emissions in REF. I would argue that Ignoring historic TAU patterns results in more plausible near-term cropland dynamics and CO2 emissions in REF.

Resources_Land_Cover_Cropland-2

Emissions_CO2_Land_Land_use_Change-2

Productivity_Landuse_Intensity_Indicator_Tau-5

@flohump flohump merged commit f00d00a into magpiemodel:develop May 26, 2021
@flohump flohump deleted the f_TAUhistfree branch June 18, 2024 16:34
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 High risk Higher risk Minor Smaller modifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants