-
Notifications
You must be signed in to change notification settings - Fork 173
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
AgroForestry: treecover on cropland and betr #644
Conversation
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.
A few documentation changes.
Seems good to me, but should be done analogous for all 3 realizations.
Please let me know if you need help with understanding the penalty implementation!
@@ -10,7 +10,7 @@ | |||
*' the sum of crop and water supply type specific land requirements: | |||
|
|||
q30_cropland(j2) .. | |||
sum((kcr,w), vm_area(j2,kcr,w)) =e= vm_land(j2,"crop"); | |||
sum((kcr,w), vm_area(j2,kcr,w)) + vm_fallow(j2) + sum(ac, v30_treecover(j2,ac)) =e= vm_land(j2,"crop"); |
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.
now you introduced fallow into the endo_apr realization. Was that on purpose?
As long as it is fixed to 0, probably doesnt matter...
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.
Just for consistency because fallow land and treecover will show up in this equation in the other realizations.
|
||
** set bii coefficients | ||
p30_treecover_bii_coeff(bii_class_secd,potnatveg) = 0; | ||
if(s30_treecover_bii_coeff = 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.
ah, now i see, this is just a switch!
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, it can be choosen flexibly. In analogy to re-/afforestation.
@@ -79,3 +93,8 @@ | |||
=e= | |||
(vm_land(j2,"crop") - sum((crop_ann30,w), vm_area(j2,crop_ann30,w))) | |||
* fm_bii_coeff("crop_per",potnatveg) * fm_luh2_side_layers(j2,potnatveg); | |||
|
|||
q30_bv_treecover(j2,potnatveg) .. vm_bv(j2,"crop_tree",potnatveg) |
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 is the set potnatveg doing?
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.
The bii coefficients are defined for two layers: forested and non-forested biomes.
@@ -50,3 +50,43 @@ p30_snv_relocation(t,j)$(p30_snv_relocation(t, j) > p30_max_snv_relocation(t,j)) | |||
*' Area potentially available for cropping | |||
p30_avl_cropland(t,j) = f30_avl_cropland(j,"%c30_marginal_land%") * (1 - p30_snv_shr(t,j)); | |||
*' @stop | |||
|
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 guess this is copy paste from a previous implementation, right? only reviewing it shortly
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. Adapted from 35_natveg.
Sure. This will be included in all 3 realizations once we agreed that the overall approach is fine. |
p30_treecover(t,j,"acx") = p30_treecover(t,j,"acx") | ||
+ sum(ac$(ord(ac) > card(ac)-s30_shift), pc30_treecover(j,ac)); | ||
|
||
pc30_treecover(j,ac) = p30_treecover(t,j,ac); |
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.
Just double checking. Here we go from 3 dimensions (t,j,ac) to 2 dimensions (j,ac). Is this right?
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 overall good to me. Please have a look at the specific comments to see my requests for modifications
|
||
pcm_land(j,land) = pm_land_start(j,land); | ||
vm_land.l(j,land) = pcm_land(j,land); | ||
|
||
pm_treecover_shr(j) = 0; | ||
pm_treecover_shr(j)$(f10_land("y2015",j,"crop") > 1e-10) = |
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 it would be more consistent to compute the treecover share in the agroforestry module. I think the existing interface pcm_land could be used for it (instead of making f10_land a new interface).
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.
We need the historic numbers for 2015. pcm_land
is the model outcome. Alternatively, we could add a time dimension to pm_land_start
(currently on 1995)
ac_est(ac) = yes$(ord(ac) <= (m_yeardiff_forestry(t)/5)); | ||
|
||
ac_sub(ac) = no; | ||
ac_sub(ac) = yes$(ord(ac) > (m_yeardiff_forestry(t)/5)); |
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 happens here and why? A new set but no other changes in the 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.
Moved from 32_forestry module.
This fits better into the ageclass module
@@ -10,7 +10,7 @@ | |||
*' the sum of crop and water supply type specific land requirements: | |||
|
|||
q30_cropland(j2) .. | |||
sum((kcr,w), vm_area(j2,kcr,w)) =e= vm_land(j2,"crop"); | |||
sum((kcr,w), vm_area(j2,kcr,w)) + vm_fallow(j2) + vm_treecover_area(j2) =e= vm_land(j2,"crop"); |
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 now fallow coming in here? this will change the default behavior of the realization, right? If so, it should be also labelled differently I think
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.
vm_fallow
is fixed to zero in this realization. Therefore, it will not change the default. I added it only for consistency with the other realizations.
*' @equations | ||
|
||
*' Total agroforestry cost. | ||
*' Cost for bioenergy trees are accounted for in the [30_crop] 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.
Please have a look at the resulting documenation. I would at least use full sentences to describe the methodology. The documentation page should read as a real documentation of the realization
# Conflicts: # CHANGELOG.md # config/default.cfg # config/scenario_fsec.csv # modules/30_crop/endo_apr21/input.gms # modules/30_crop/penalty_apr22/input.gms # modules/30_crop/rotation_apr22/input.gms
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.
More comments not always full sentences 😅 but I guess this might be a matter of preference.
The only thing I again thought of was the alignment of fallow in bii and som regards. The difference might be not big, but it might be still good to align these? But this could be also done when I do the changes to the dynamic realization to align them to the new carbon stocks in a couple of weeks.
vm_fallow(j2) =l= | ||
vm_land(j2,"crop") * s29_fallow_max; | ||
|
||
*' Fallow land biodiversity value is based on perennial crops. |
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, but still than we shoudl adress this similarly in the SOM realization:
https://github.com/flohump/magpie/blob/1774fcfba8467d6695b50af1c395efc77672beb1/modules/59_som/cellpool_jan23/preloop.gms#L71
Here we use maize as reference. Maybe we can switch it to "oilpalm" (which is the only perenial we have in som module.
if we change it, for both dynamic realizations it has to be changed ^^'
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
🐦 Description of this PR 🐦
This PR adds tree cover on cropland and bioenergy trees as two different AgroForestry systems to MAgPIE
To intergrate cropland tree cover properly into the existing model, the following changes are applied:
29_cropland
, accouting for crop area, fallow cropland and tree cover on cropland30_crop
renamed to30_croparea
, which now only accounts for crop area.29_ageclass
has been renamed to28_ageclass
30_crop
to29_cropland
30_crop/penalty
and30_crop/rotation
realizations have been merged into a singe30_croparea/detail_apr24
realization30_crop/endo_apr21
crop realization has been renamed to30_croparea/simple_apr24
29_cropland
has two realizations:detail_apr24
andsimple_apr24
(default)todo: include input files for 30_croparea in additional_data.tgz.
todo: emisCO2 in magpie4 reporting
Changelog
changed
29_cropland
just before30_croparea
30_crop
renamed to30_croparea
, which now only accounts for crop area.30_crop
to29_cropland/detail_apr24
penalty_apr22
androtation_apr22
have been merged into a single30_croparea/detail_apr24
realization30_crop/endo_apr21
realization has been moved to30_croparea/simple_apr24
added
29_cropland
accounting for crop area, fallow cropland and tree cover on cropland with two realizations:detail_apr24
andsimple_apr24
(default) .pm_land_hist
with historic land use patternsv32_land_missing_ndc
fixed
🔧 Checklist for PR creator 🔧
Label pull request from the label list.
Self-review own code
magpie4
R library has been updated accordingly and backwards compatible where necessary.scenario_config.csv
has been updated accordingly (important ifdefault.cfg
has been updated)Document changes
CHANGELOG.md
goxygen::goxygen()
and verify the modified code is properly documentedPerform test runs
Rscript start.R --> "compilation check"
Rscript start.R --> "test runs"
Rscript start.R --> "test runs"
📉 Performance changes 📈
🚨 Checklist for reviewer 🚨
CHANGELOG
is updated correctly