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

Added primary and secondary other land adjustment to BII disaggregattion #415

Merged
merged 11 commits into from
Jul 27, 2022

Conversation

pvjeetze
Copy link
Contributor

@pvjeetze pvjeetze commented Jul 8, 2022

🐦 Purpose of this PR 🐦

  • New BII disaggregation with primary and secondary other land specification.
  • Also includes changes/rewrite of magpie4::PrimSecdOtherLand

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

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

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

@pvjeetze pvjeetze added enhancement New feature or request Low risk Low risk labels Jul 8, 2022
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 check whether the dependency on mrcommons is really needed?

library(magpie4)
library(luscale)
library(madrat)
library(mrcommons)
Copy link
Member

Choose a reason for hiding this comment

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

what is madrat and mrcommons being used for here? Output scripts should not use any mr-packages as this would limit their use to machines with a full madrat installation including all relevant sources, which is currently only available on the PIK cluster.

Copy link
Member

Choose a reason for hiding this comment

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

(toolAggregate can be used, but no calc-, read- or download- functions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was inherited from the original script. It also e.g. occurs in the LUH disaggregation script. While this is, of course, not a justification, maybe we can fix this in a different PR? It might require some rework of the package structure. In this particular case mrcommons::toolCell2isoCell is used in line 64

Copy link
Member

Choose a reason for hiding this comment

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

ok, we can fix this later as the use of a tool-function (while not optimal) is not that of an issue like the use of a read- or calc- function would be

Copy link
Contributor

@flohump flohump 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

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.

Since this is an improvement of the existing BII script, why don't you replace the old script but introduce a new one?
I would suggest to replace the existing one rather than introducing a parallel BII_adjusted.

@pvjeetze pvjeetze merged commit 5fd16eb into magpiemodel:develop Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Low risk Low risk
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants