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

New employment module and switches for regional factor requirements (for crop and livst) #397

Merged
merged 40 commits into from
Jun 15, 2022

Conversation

deleip
Copy link
Contributor

@deleip deleip commented May 25, 2022

🐦 Purpose of this PR 🐦

  • This PR adds an employment module (36_employment), which calculates agricultural employment (for crop and livestock production) based on total labor costs. Further, switches to use regional factor requirements for crops (for realizations per_ton_fao_may22, sticky_feb18, sticky_labor) and regional factor requirements regression for livestock are included (c38_fac_req and c70_fac_req_regr).

🔧 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
  • 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 : 20 mins
    • This PR's default : 22 mins
  • 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.

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

  • calibration runs (The default setting cfg$best_calib <- TRUE was used only for the sticky factor costs realization, and cfg$best_calib <- FALSE was used for mixed and per_ton, to avoid cropland levels not matching the validation data. For global factor requirements the calibration values from before are kept, as this corresponds to the old implementation.)
    Screenshot from 2022-06-13 21-00-25
    Screenshot from 2022-06-13 21-05-38
  • old default vs. new default:
    Screenshot from 2022-06-13 22-25-22
    Screenshot from 2022-06-13 22-25-40
  • new indicators (ag. employment is calibrated for per_ton_fao with regional factor requirements):
    Screenshot from 2022-06-13 22-27-53
    Screenshot from 2022-06-13 22-28-58
    Screenshot from 2022-06-13 22-28-47

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

@deleip deleip marked this pull request as draft May 25, 2022 13:31
@deleip deleip added Major Substantial modifications High risk Higher risk labels May 25, 2022
@deleip deleip changed the base branch from master to develop May 25, 2022 13:39
@deleip deleip changed the title [draft] New employment module and switches for regional factor requirements (for crop and livst) New employment module and switches for regional factor requirements (for crop and livst) Jun 13, 2022
@deleip deleip marked this pull request as ready for review June 13, 2022 20:54
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.

Methodologically all looks great. Smaller comments to address.


q36_employment(i2) .. v36_employment(i2)
=e= (vm_cost_prod_crop(i2,"labor") + vm_cost_prod_livst(i2,"labor") + sum(ct,p36_nonmagpie_labor_costs(ct,i2))) *
(1 / sum(ct,f36_weekly_hours(ct,i2)*s36_weeks_in_year*p36_hourly_costs(ct,i2)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is doesn't influence the optimization, I'm wondering it should better go to the postsolve instead. Maybe this is a question to @tscheypidi here.

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 think we had shortly discussed this a while ago and decided to have it as an equation, as we might make changes in the future for which it then could influence the optimization. But happy to move it to the postsolve if that would be best for now.

modules/36_employment/exo_may22/preloop.gms Outdated Show resolved Hide resolved
modules/36_employment/exo_may22/presolve.gms Show resolved Hide resolved
modules/38_factor_costs/sticky_feb18/declarations.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.

all good.

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.

some minor comments. Two additional questions:

  1. Did you check that magpie4 and the output scripts are compatible to your renamings in the code?
  2. Your runs (also the default run) show significantly lower runtimes to what is reported in other PRs. Can you explain where this is coming from?

# * (sticky_feb18, global fac req) "calibration_H12_sticky_feb18_28May22.tgz"
# * (sticky_feb18, regional fac req) "calibration_H12_sticky_feb18_reg_13Jun22.tgz"
# * (sticky_labor, global fac req) "calibration_H12_sticky_feb18_28May22.tgz"
# * (sticky_labor, regional fac req) "calibration_H12_sticky_feb18_reg_13Jun22.tgz"
Copy link
Member

Choose a reason for hiding this comment

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

this seems to become rather complex, labor intensive having so many different combination of calibration factors. I would think that this is ok for now, but we keep that in mind and think about how we can avoid further extensions of this list or even find solutions for a reduction again.

config/default.cfg Show resolved Hide resolved

*' @equations

* excluding labor costs for crop residues (as this is not include in ILO empl. data)
Copy link
Member

Choose a reason for hiding this comment

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

what means "ILO empl."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

employment data from the International labour organization, which is the data source we use. I'll change "empl." to "employment"

modules/36_employment/exo_may22/equations.gms Show resolved Hide resolved

*' @title Employment

*' @description This module calculates agricultural employment in
Copy link
Member

Choose a reason for hiding this comment

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

The module description should be a bit more detailed and should be seen as a "module contract" which defines what this module is responsible for and what information it expects in order to fulfill its role.

I should contain the following components: 1) Description of the purpose of the module, 2) statement of what the module will provide back to the model 3) statement of what information it expects from other modules, 4) (if applicable) statement of what is guaranteed to be unaffected by the module (e.g. "no effect on optimization" if this is a general characteristic of the module not just some of its realizations).

The module description is important to define responsibilities and distinguish between responsibilities of the module and the rest model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If added some more detail. So far, nothing is provided back to the model, and the optimization is not changed. However this is only true in the current implementation and might change with future updates, so I didn't include this as a general statement. Let me know if this is detailed enough now

@@ -16,6 +16,7 @@ vm_cost_prod_crop(i,req) Regional factor costs of capital and labor for p
parameter
p38_cost_share(t,i,req) Capital and labor shares of the regional factor costs for plant production (1)
p38_share_calibration(i) Summation factor used to calibrate calculated capital shares with historical values (1)
i38_fac_req(t_all,i,kcr) Regional or global factor requirements (USD05MER per tDM)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
i38_fac_req(t_all,i,kcr) Regional or global factor requirements (USD05MER per tDM)
i38_fac_req(t_all,i,kcr) Factor requirements (USD05MER per tDM)

I would remove the "regional or global" as the values are always reported on the regional level. The distinction is that in one setup it is assumed that all regional values are identical and the other case it is assumed to have regional differences

@@ -28,6 +28,8 @@ parameters
p38_share_calibration(i) Summation factor used to calibrate calculated capital shares with historical values (1)

p38_croparea_start(j,w,kcr) Agricultural land initialization area (mio. ha)

i38_fac_req(t_all,i,kcr) Regional or global factor requirements (USD05MER per tDM)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
i38_fac_req(t_all,i,kcr) Regional or global factor requirements (USD05MER per tDM)
i38_fac_req(t_all,i,kcr) Factor requirements (USD05MER per tDM)

@@ -34,6 +34,8 @@ parameters

i38_ces_shr(j,kcr) Share parameter for CES function (1)
i38_ces_scale(j,kcr) Scaling factor for total factor productivity (1)

i38_fac_req(t_all,i,kcr) Regional or global factor requirements (USD05MER per tDM)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
i38_fac_req(t_all,i,kcr) Regional or global factor requirements (USD05MER per tDM)
i38_fac_req(t_all,i,kcr) Factor requirements (USD05MER per tDM)

@deleip
Copy link
Contributor Author

deleip commented Jun 15, 2022

some minor comments. Two additional questions:

1. Did you check that `magpie4` and the output scripts are compatible to your renamings in the code?

2. Your runs (also the default run) show significantly lower runtimes to what is reported in other PRs. Can you explain where this is coming from?
  1. Yes, I checked magpie4 and the output scripts
  2. I believe it comes from the newer GAMS version.

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.

looks good

@deleip deleip merged commit c146e0a into magpiemodel:develop Jun 15, 2022
@deleip deleip deleted the f_employment branch November 2, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High risk Higher risk Major Substantial modifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants