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

labour productivity module + new sticky factor cost realization + bugfixes #332

Merged
merged 67 commits into from
Oct 21, 2021

Conversation

flohump
Copy link
Contributor

@flohump flohump commented Oct 7, 2021

🐦 Purpose of this PR 🐦

Added

  • 37_labor_prod labor productivity module with two realizations: off and exo
  • 38_factor_costs new realization "sticky_labor", based on "sticky_feb18" but accounting for changes in labor productivity
  • scripts scripts/output/extra/disaggregation_LUH2.R script for exporting spatial output in LUH2 format (NetCDF)

Fixed

  • 35_natveg fixed land protection to SSP2 default (WDPA) for historic period
  • 15_food New iteration needs to be started before setting food prices for curr_iter15
  • scripts scripts/output/extra/highres.R bugfixes
  • 38_factor_costs units in sticky_feb18

Removed

  • scripts scripts/start/extra/highres.R

Changed

  • scripts scripts/start/projects/project_LAMACLIMA.R -> scripts/start/projects/project_LAMACLIMA_WP4.R

🔧 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: PR331_default_old
    • Default run based on the current version of the fork from which PR is made: PR331_default_new

Costs-4
Emissions_CO2_Land_Land_use_Change-25
Productivity_Landuse_Intensity_Indicator_Tau-32
Resources_Land_Cover_Cropland-34

  • 📉 Performance loss/gain from current default behavior 📈
    • Current develop branch default : 0.46 h
    • This PR's default : 0.54 h

Unknown-63

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

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

…e into f_labourprod

# Conflicts:
#	CHANGELOG.md
…e into f_labourprod

# Conflicts:
#	modules/14_yields/managementcalib_aug19/preloop.gms
@flohump flohump changed the title labour productivity module + extended stick factor cost realization labour productivity module + new sticky factor cost realization + bugfixes Oct 19, 2021
@flohump flohump added enhancement New feature or request High risk Higher risk labels Oct 19, 2021
@flohump flohump marked this pull request as ready for review October 19, 2021 13:08
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.

Can you please check whether it could be sufficient to only copy one of the avl_land files to the output folder as duplication of these large files leads to serious memory consumption

@@ -1385,6 +1420,7 @@ cfg$files2export <- list()
# Files that should be copied before MAgPIE is started
cfg$files2export$start <- c("input/info.txt",
"modules/10_land/input/avl_land_t_0.5.mz",
"input/avl_land_full_t_0.5.mz",
Copy link
Member

Choose a reason for hiding this comment

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

is it necessary to copy both avl_land files to the output folders? I guess both are quite similar and big. If possible, reduce it to one file

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 see your point. But this file is needed for scripts/output/extra/disaggregation_LUH2.R. I think we should switch soon to using avl_land_full_t in MAgPIE. The only difference is that pasture is split into rangeland and pasture, and that other land is split into primary and secondary other land. As soon as we switch, only avl_land_full_t will be needed. In this concrete case, the file is 5 MB, which is small compared to other files we copy by default such as f50_AtmosphericDepositionRates_0.5.mz : 32 MB. Anyways, I could imagine a more space-efficent approch for files needed in post-processing. Instead of copying these files into each output folder, these files could be copied once to scripts/output/data/[name_of_celluar_input]. E.g. scripts/output/data/rev4.64_h12_477f2095_cellularmagpie_c200_MRI-ESM2-0-ssp370_lpjml-4b917a03. The output scripts know which celluar input was used (cfg object) and can take the needed data from the respective folder in scripts/output/data/.

Copy link
Member

Choose a reason for hiding this comment

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

so, if avl_land_full_t_0.5.mz is just a more detailed version of avl_land_t_0.5.mz, why does the latter still have to be copied to the output folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because both disaggregation.R and disaggregation_LUH2.R use avl_land_t_0.5.mz. In disaggregation_LUH2.R, avl_land_full_t_0.5.mz is just used in an additional step.
As an alternative to exporting avl_land_full_t_0.5.mz by default, I can include the export of avl_land_full_t_0.5.mz just in my start script.

Copy link
Member

@bodirsky bodirsky 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 in demand module and factor cost module seem to have been considered.
My review only concerns labout productivity module and demand bugfixes. They seem alright!

modules/38_factor_costs/sticky_labor/presolve.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.

looks good

@flohump flohump merged commit cb41ccc into magpiemodel:develop Oct 21, 2021
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.

3 participants