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

Inclusion of AQUASTAT calibration factors for yields #457

Merged
merged 38 commits into from
Oct 13, 2022

Conversation

vartika271987
Copy link
Contributor

@vartika271987 vartika271987 commented Oct 3, 2022

🐦 Purpose of this PR 🐦

Inclusion of AQUASTAT rainfed vs irrigation calibration factors as a switch to improve yield ratios

🔧 Checklist for PR creator 🔧

    • 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
    image
    image
    image
    image
    image

AAI

irrigYieldsAFTERcalib

rainfedYieldsAFTERcalib

  • High risk

    • Default run from the current develop branch (2909_nocalib)
    • Default run based on the current version of the fork from which PR is made (2909_yescalib)
  • 📉 Performance loss/gain from current default behavior 📈

    • Current develop branch default : 58 mins
    • This PR's default : 60 mins
    • Default version: 0.40, new version: 0.45
  • 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.

  • In case of updated cellular input tgz file in default.cfg: scenario_config.csv has been updated accordingly (rcp1p9, rcp2p6 etc)

  • 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%

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

@vartika271987 vartika271987 added enhancement New feature or request High risk Higher risk labels Oct 3, 2022
Note moved.
CHANGELOG.md Outdated Show resolved Hide resolved
config/default.cfg Outdated Show resolved Hide resolved
config/default.cfg Outdated Show resolved Hide resolved
@@ -899,12 +901,15 @@ cfg$gms$c37_labor_uncertainty <- "ensmean" # default = "ensmean"
# * (per_ton_fao_may22) factor costs fixed per ton
# * (sticky_feb18) factor costs including investments in capital
# * (sticky_labor) based on sticky_feb18 + labor productivity factor included
# NOTE: It is recommended to recalibrate the model when changing this setting!
# NOTE: It is recommended to use best_calib only for sticky factor cost realizations!
Copy link
Member

Choose a reason for hiding this comment

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

are we still sure about that? Perhaps skip that line?

Copy link
Member

Choose a reason for hiding this comment

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

@k4rst3ns @emolinab : Should we remove this line w.r.t. "best_calib"? If we are unsure about it, we should maybe better leave it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

where exactly?

Copy link
Member

Choose a reason for hiding this comment

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

It refers to this chunk:

# ***---------------------    38_factor_costs    -------------------------------
# * Make sure you use the corresponding calibration file to the selected realization
# * Please, check details in input settings.
# * (per_ton_fao_may22)    factor costs fixed per ton
# * (sticky_feb18)         factor costs including investments in capital
# * (sticky_labor)         based on sticky_feb18 + labor productivity factor included
# NOTE: It is recommended to recalibrate the model when changing this setting!
# NOTE: It is recommended to use best_calib only for sticky factor cost realizations!
cfg$gms$factor_costs <- "per_ton_fao_may22"        # default = per_ton_fao_may22

And the question is whether we leave the line: "# NOTE: It is recommended to use best_calib only for sticky factor cost realizations!". Jan was not sure if this still applies or not. Kristine and I understood that this was the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please change it to: "In case the maximum number of iterations is reached without convergence, please consider to use the best_calib setting"

modules/14_yields/managementcalib_aug19/preloop.gms Outdated Show resolved Hide resolved
modules/18_residues/off/not_used.txt Outdated Show resolved Hide resolved
modules/42_water_demand/all_sectors_aug13/equations.gms Outdated Show resolved Hide resolved
Copy link
Member

@xwangatpik xwangatpik left a comment

Choose a reason for hiding this comment

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

The test results in general look reasonable, also for China. Just a remark that the ir-rf calibration makes the rainfed yields higher and the irrigated yields lower for India. How can this give a larger amount of agricultural water use in the run with ir-rf calibration?

FelicitasBeier and others added 2 commits October 6, 2022 16:22
Co-authored-by: Jan Dietrich <dietrich@pik-potsdam.de>
Co-authored-by: Jan Dietrich <dietrich@pik-potsdam.de>
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.

From the calculation aspect all looks fine from my side.

config/default.cfg Outdated Show resolved Hide resolved
modules/18_residues/off/not_used.txt Outdated Show resolved Hide resolved
@k4rst3ns
Copy link
Member

k4rst3ns commented Oct 7, 2022

The test results in general look reasonable, also for China. Just a remark that the ir-rf calibration makes the rainfed yields higher and the irrigated yields lower for India. How can this give a larger amount of agricultural water use in the run with ir-rf calibration?

Hey @xiaoxi: I am unsure with the actual irrgitaed yields will tell us something on regional scales as they are always weighted by the actual crop pattern which can lead to different results. E.g. imagine MAgPIE is now doing more irrigation at places with not so high yield than before. If so irrigated yield of IND will be dropping.
Maybe look on Yields (after calibration) - this is weighted with always the same weights and therefor is a more stable variable to look at if we want to compare yields across runs maybe?

@FelicitasBeier
Copy link
Member

The test results in general look reasonable, also for China. Just a remark that the ir-rf calibration makes the rainfed yields higher and the irrigated yields lower for India. How can this give a larger amount of agricultural water use in the run with ir-rf calibration?

I have added the indicator "Yields after calibraiton" for both rainfed and irrigated yields. I think this explains why there is an incentive to irrigate (i.e. higher withdrawals and higher irrigated area in India). Please confirm.

@xwangatpik
Copy link
Member

All good. Thanks for the clarification, Feli and Kristine :)

@xwangatpik xwangatpik self-requested a review October 9, 2022 06:27
Copy link
Member

@FelicitasBeier FelicitasBeier left a comment

Choose a reason for hiding this comment

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

My comments have been resolved.

@@ -899,12 +901,15 @@ cfg$gms$c37_labor_uncertainty <- "ensmean" # default = "ensmean"
# * (per_ton_fao_may22) factor costs fixed per ton
# * (sticky_feb18) factor costs including investments in capital
# * (sticky_labor) based on sticky_feb18 + labor productivity factor included
# NOTE: It is recommended to recalibrate the model when changing this setting!
# NOTE: It is recommended to use best_calib only for sticky factor cost realizations!
Copy link
Member

Choose a reason for hiding this comment

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

@k4rst3ns @emolinab : Should we remove this line w.r.t. "best_calib"? If we are unsure about it, we should maybe better leave it out.

modules/14_yields/managementcalib_aug19/preloop.gms Outdated Show resolved Hide resolved
modules/18_residues/off/not_used.txt Outdated Show resolved Hide resolved
modules/42_water_demand/all_sectors_aug13/equations.gms Outdated Show resolved Hide resolved
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.

I just found one line where the goxygen comment prefix is missing (see my suggested change and please incorporate it). Other than that it looks ready to go for me.

modules/14_yields/managementcalib_aug19/preloop.gms Outdated Show resolved Hide resolved
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.

just a very tiny edit in the description requested.

modules/14_yields/module.gms Show resolved Hide resolved
@FelicitasBeier FelicitasBeier merged commit e39f5c3 into magpiemodel:develop Oct 13, 2022
xwangatpik added a commit to xwangatpik/magpie that referenced this pull request Feb 11, 2023
…secCHA

 Inclusion of AQUASTAT calibration factors for yields magpiemodel#457
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request High risk Higher risk
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants