-
Notifications
You must be signed in to change notification settings - Fork 178
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
Introduce simple soil carbon management (scm) option #774
base: develop
Are you sure you want to change the base?
Conversation
@@ -32,14 +32,15 @@ | |||
+ vm_cost_timber(i2) | |||
+ vm_cost_hvarea_natveg(i2) | |||
+ vm_cost_processing(i2) | |||
+ sum(cell(i2,j2), vm_cost_scm(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.
did you check the cost scripts in magpie4 whether they correctly include these costs? (the magpie4 script sometimes requires a categorization of the costs into one-off investments and yerarly costs)
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 case of questions wrt these scripts, you can ask edna or david, 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.
no cost not included yet in magpie4 - I will ask for help and commit to magpie4.
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.
When done, please also increase the magpie4 R library version number in the DESC file.
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 addressing my comments.
Fine to merge for me.
Changes in magpie4 library (costs) are still to do.
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 these improvements.
Looks good overall.
I have some minor comments and requests.
Can you add results for the static_jan19
realisation (current default).
And for overview, it would be great to have global cropland and land-use change CO2 emissions.
@@ -1807,12 +1807,12 @@ cfg$gms$s58_fix_peatland <- 2020 # def = 2020 | |||
|
|||
# ***------------------------- 59_som ------------------------------------- | |||
# * (static_jan19): static soil carbon loss for cropland | |||
# * (cellpool_aug16): dynamic soil organic matter pool on cellular level | |||
# * (cellpool_jan23): dynamic soil organic matter pool on cellular level | |||
# * with updated, regionalized stock change factors (IPCC guidelines 2019) | |||
cfg$gms$som <- "static_jan19" # def = static_jan19 |
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 we make cellpool_jan23
the new default?
This would make the setup of coupled REMIND-MAgPIE runs with SCM much easier.
I guess that with s59_scm_target <- 0
the results are comparable to the current default?
@@ -32,14 +32,15 @@ | |||
+ vm_cost_timber(i2) | |||
+ vm_cost_hvarea_natveg(i2) | |||
+ vm_cost_processing(i2) | |||
+ sum(cell(i2,j2), vm_cost_scm(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.
When done, please also increase the magpie4 R library version number in the DESC file.
@@ -12,7 +12,8 @@ | |||
|
|||
q59_som_target_cropland(j2) .. | |||
v59_som_target(j2,"crop") | |||
=e= (sum((kcr,w), vm_area(j2,kcr,w) * i59_cratio(j2,kcr,w)) | |||
=e= (sum((kcr,w), vm_area(j2,kcr,w) * i59_cratio(j2,kcr,w)) * | |||
(1 + sum(ct, i59_scm_target(ct,j2)) * (i59_ratio_scm(j2) - 1)) |
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 don't understand i59_ratio_scm(j2) - 1)
.
Can you add a brief explanation?
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.
Wil change to something like
(sum((kcr,w), vm_area(j2,kcr,w) * i59_cratio(j2,kcr,w)) +
sum((kcr,w), vm_area(j2,kcr,w) * sum(ct, i59_scm_target(ct,j2))
i59_cratio(j2,kcr,w)) * (i59_ratio_scm(j2) - 1))
* 3: In gms::check_config(cfg, extras = c("info", "repositories", ... : | ||
* Config looks different when stored via saveConfig and loaded via loadConfig! | ||
* 4: In gms::check_config(cfg, extras = c("info", "repositories", ... : | ||
* Config looks different when stored via saveConfig and loaded via loadConfig! |
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 happened here? There should be no warnings committed to main.gms and we need to check whether the warning indicates a relevant problem here.
main.gms
Outdated
@@ -278,7 +288,7 @@ $setglobal awms ipcc2006_aug16 | |||
$setglobal ghg_policy price_aug22 | |||
$setglobal maccs on_aug22 | |||
$setglobal peatland v2 | |||
$setglobal som static_jan19 | |||
$setglobal som cellpool_jan23 |
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.
non-default setting
@@ -12,7 +12,8 @@ | |||
|
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.
perhaps add a sentence to the documentation, that the equilibrium value can be boosted via SCM?
; | ||
|
||
* The implementation of soil carbon management on cropland refers to a diverse set of |
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 am wondering whether this comment should only show up here or should be also part of the in-code documentation.
m_sigmoid_time_interpol(i59_scm_scenario_fader,s59_scm_scenario_start,s59_scm_scenario_target,0,1); | ||
); | ||
|
||
* Country switch to determine countries for which certain policies shall be applied. |
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 like the code will still be put into the goxygen documentation, but this comment will not show up as part of the documentation (maybe instead as a comment in the code? I am honestly not sure).
Most likely this should be either also a goxygen comment or the @Stop from line 111 should happen earlier.
please build the documentation and have a look at how it looks (also related to the other comments above)
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 a lot @k4rst3ns for reworking the whole realisation. I only have minor comments to add to the previous comments made by the others.
cfg$gms$s59_fader_functional_form <- 1 # def = 1 | ||
|
||
# * Sigmoid fader for minimum area share of SCM on total cropland at cluster level | ||
# * Minimum area share of SCM on total cropland in target year |
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.
Maybe consider to add examples of plausible values here.
🐦 Description of this PR 🐦
This PR
🔧 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
/p/projects/magpie/users/karstens/MAGPIEMODEL/magpie_socm/output with
default_0pX_SCM01[_Y]
with X being the scm target share, and Y additional settings like
1p5
degree goal ornocc
runsCheck logs for errors/warnings
Soil Carbon reacts to scm settings the way expected

Cropland shows small reduction effects due to higher costs

Emissions only slightly reacts

🚨 Checklist for reviewer 🚨
CHANGELOG
is updated correctly