-
Notifications
You must be signed in to change notification settings - Fork 164
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
New protected area baseline and option for restoration in future land conservation options #396
Conversation
…f_WDPABaseline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for including these improvements!
In general looks fine to me.
I have some smaller comments for clarification.
Consider adding a sentence for the new interface pm_land_conservation
in the module contract for 22_land_conservation.
We still need to agree on a new value for cfg$gms$s13_max_gdp_shr
.
Disclaimer: I had several bilateral talks with @pvjeetze regarding the suggested model changes in this PR.
config/default.cfg
Outdated
cfg$gms$policy_countries22 <- all_iso_countries | ||
|
||
# * Whether land restoration is carried out for future options | ||
cfg$gms$s22_restore_land <- 0 # def = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not activated by default?
Spell out all options: 0: off; 1: on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, I also prefer to have this actually activated by default. This also gives support to suggested changes below of simplifying the condition for restoration in the reference period 1995-2020. If cfg$gms$s22_restore_land <- 1
is the default anyway, we may not need the additional condition of making sure that PAs are restored in the ref period based on the historic data
config/default.cfg
Outdated
|
||
# Land conservation policy | ||
# * Baseline: | ||
# * (WDPA) All areas under legal protection from 1995 until 2020 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be very helpful to have some aggregate global numbers here.
Basically, the numbers you have in the desc of the PR.
*' areas). Natural vegetation (natveg) and grassland ('past') within protected areas cannot | ||
*' be converted to other land types. On top of the WDPA baseline protection, there are future | ||
*' options to protect different conservation priority areas such as biodiversity hotspots (BH), | ||
*' centers of plant diversity (CBD), Intact Forest Landscapes (IFL) and last of the wild (LW). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add reference: ", taken from Brooks et al 2006" http://science.sciencemag.org/content/313/5783/58
*' | ||
*' @limitations Land cover in the WDPA baseline data is estimated based on ESA-CCI | ||
*' land-use/land-cover maps from 1995 to 2020, while land pools in MAgPIE are intialised | ||
*' based on LUH2v2 data. This leads to slight mismatches in some areas. Where the area under |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add that we calibrate the LUH2v2 LU patterns to match FAO numbers for forest.
|
||
table f22_wdpa_baseline(t_all,j,land) Initial protected area as derived from WDPA until 2020 (mio. ha) | ||
$ondelim | ||
$include "./modules/22_land_conservation/input/wdpa_baseline.cs3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This files is included in "rev4.68_h12_fd712c0b_cellularmagpie_c200_MRI-ESM2-0-ssp370_lpjml-8e6c5eb1.tgz"
?
No update of input tgz file vector in default.cfg needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I will still have to set up a new preprocessing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will set up a revision 4.71
today afternoon
CHANGELOG.md
Outdated
@@ -45,6 +47,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
- **scripts** Extended dissagregation.R script to replace single "past" land class by LHU range and pastr classes when grassland_apr22 realization is used. | |||
- **52_carbon** added land carbon sink adjustment factors, needed in R post-processing | |||
- **core** macros for linear and sigmoidal time interpolation | |||
- **22_land_conservation** added new module and realisation for land protection. The realisation also includes a new WDPA initialisation data set (from 1995 to 2020) for protected areas under legal protection and meeting IUCN and CBD protected area definitions (including IUCN categories Ia, Ib, III, IV, V, VI and 'not assigned' but legally designated). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention the new interface pm_land_conservation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and you can perhaps skip the IUCN categories. The changelog should primarily help to identify what changes happened when, the details should be part of the module/realization documentation
* Land restoration | ||
* ------------------- | ||
|
||
if(s22_restore_land = 1 OR m_year(t) <= sm_fix_SSP2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That means irrespective of s22_restore_land
there is land restoration between 1995 and 2020?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you confirm that this is the intended behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If yes, please clarify in the default.cfg that s22_restore_land
has only effects after the year given in sm_fix_SSP2
has passed.
In addition, you could consider introducing an internal switch instead of using sm_fix_SSP2
. @tscheypidi What is your suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using sm_fix_SSP2
here is correct as the description of this switch reads " Year until which all parameters are fixed to SSP2 values"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I formulated the conditions as such, as I thought cfg$gms$s22_restore_land <- 0
might become the default. I also think, cfg$gms$s22_restore_land <- 1
would be even better as the default. This would mean I can remove m_year(t) <= sm_fix_SSP2
, which made sure that land conservation in the reference period followed the WDPA data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work! I put some comments/change requests. In addition, please also compile the goxygen document and have a look at it. I think it might require some rework / more details in some parts.
CHANGELOG.md
Outdated
@@ -45,6 +47,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
- **scripts** Extended dissagregation.R script to replace single "past" land class by LHU range and pastr classes when grassland_apr22 realization is used. | |||
- **52_carbon** added land carbon sink adjustment factors, needed in R post-processing | |||
- **core** macros for linear and sigmoidal time interpolation | |||
- **22_land_conservation** added new module and realisation for land protection. The realisation also includes a new WDPA initialisation data set (from 1995 to 2020) for protected areas under legal protection and meeting IUCN and CBD protected area definitions (including IUCN categories Ia, Ib, III, IV, V, VI and 'not assigned' but legally designated). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and you can perhaps skip the IUCN categories. The changelog should primarily help to identify what changes happened when, the details should be part of the module/realization documentation
config/default.cfg
Outdated
# * (WDPA) All areas under legal protection from 1995 until 2020 | ||
# * (IUCN categories Ia, Ib, II, III, IV, V, VI + 'not assigned') | ||
# * as derived from WDPA (land cover estimated based on ESA CCI) | ||
# * Future options: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what means "future options"? Do you mean that you put something on top of WDPA for future time steps? If so, I would remove the separation into baseline and future options as this is (at least for me) for confusing than helpful
# * Start and target year of land conservation option ('c22_protect_scenario'). | ||
# * Defines when full protection based on 'future options' is reached, using a | ||
# * sigmoidal trajectory. NOTE: This switch is only relevant | ||
# * if one of the 'future options' (e.g. BH) is chosen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, now I understand why you make this distinction above into future and baseline. Is it correct, that WDPA is always active, no matter what setting one chooses? If so, it might perhaps make sense to have a switch which only containts the scenarios which are put on top of WDPA and have a setting "none" if you want to just run WDPA. This might make it clearer how the different switches interact with each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes in the current version WDPA is always active, changing the option to "none" would entail quite changes to the way it is set up right now. "WDPA" is simply a set item in the the input data, which is set to zero. Making it quite efficient in the code structure. Changing it to "none" would require a own $ifthen section to allow for this option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now done some rework of the input data mainly for easier use during postprocessing, but also to make clearer the difference between the WDPA baseline and future options for conservation in conservation priority areas. protect_area.cs3
is now replaced with consv_prio_areas.cs3
created with the new function calcConservationPriority
in mrland
. This also allowed me to simplify/rework the code a bit. Please check again.
*' @equations | ||
|
||
*' Natural land conservation | ||
*' Total natural land cannot be smaller than total natural land conservation target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use whole sentences for the goxygen document. And please also run goxygen to check how the documentation of the new module looks like
=g= | ||
sum(ct, p22_min_forest(ct,j2)); | ||
|
||
q22_min_other(j2) .. vm_land(j2,"other") =g= sum(ct, p22_min_other(ct,j2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is that a separate constraint? Can't it be part of q22_natveg_conservation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, the documentation comments could be a bit more detailed/explanatory here.
q22_natveg_conservation(j2) .. | ||
sum(land_natveg, vm_land(j2,land_natveg)) | ||
=g= | ||
sum((ct,land_natveg,consv_type), pm_land_conservation(ct,j2,land_natveg,consv_type)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, it seems that you have a mix of constraints, some of which are set here in the land conservation module and others which are set in the corresponding land modules (based on pm_land_conservation
. Please choose only one of the two options: Either do all constraints here (in which case pm_land_conservation
is not needed anymore) or do all in the corresponding land modules (in which case you don't need vm_land
anymore as input).
I don't have a clear preference, could only imagine that in some cases the treatment of land conservations constraints might be land realization specific in which case option two would be preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can easily move q22_natveg_conservation(j2)
to the natveg module. Adding NPI/NDC to the land conservation module was also something that I discussed with Florian.. @flohump what would you say? Shall I move the NPI/NDC equation back to the natveg module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, we discussed this previously. But looking at the code again, I see Jan's point. It's better to have the constraints in one module, i.e. moving q22_natveg_conservation back to the natveg module. With NPI/NDC I'm a bit torn because content-wise it fits to the new 22_land_conservation module. But reading-in the NPI data in 22_land_conservation module and protecting the area in 35_natveg would require another interface. Therefore, we could also move back the NPI/NDC code to 35_natveg. That means the new module 22 would only provide the interface pm_land_conservation without having any constraints.
* Land restoration | ||
* ------------------- | ||
|
||
if(s22_restore_land = 1 OR m_year(t) <= sm_fix_SSP2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using sm_fix_SSP2
here is correct as the description of this switch reads " Year until which all parameters are fixed to SSP2 values"
Co-authored-by: Florian Humpenöder <humpenoeder@pik-potsdam.de>
Co-authored-by: Jan Dietrich <dietrich@pik-potsdam.de>
…f_WDPABaselineNew
I've updated the code, please see whether, I have addressed all requests. The updated preprocessing will follow this afternoon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me. Just some additional questions / change requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked again through all changes and did not find any inconsistencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
🐦 Purpose of this PR 🐦
🔧 Checklist for PR creator 🔧
📉 Performance loss/gain from current default behavior 📈
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 (usegoxygen::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%
New preprocessing has to be set up still
🚨 Checklist for RSE reviewer 🚨
CHANGELOG
is updated correctly🚨 Checklist for MAgPIE reviewer 🚨
CHANGELOG
is updated correctly🔴 lines: default in current develop. 🟢 lines: default of this PR