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

Managed pastures tau #372

Closed
wants to merge 8 commits into from
Closed

Conversation

mppalves
Copy link
Contributor

🐦 Purpose of this PR 🐦

  • Managed pastures implementation PR 1: Added a new tau dimension for managed pastures.

🔧 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: /p/projects/landuse/users/pedrosa/MAgPIE/develop_clean/output/default_2022-01-30_17.04.22
    • Default run based on the current version of the fork from which PR is made: /p/projects/landuse/users/pedrosa/MAgPIE/MAgPIE_clean/output/default_2022-01-30_16.11.39
  • 📉 Performance loss/gain from current default behavior 📈

    • Current develop branch default : 32 mins
    • This PR's default : 33 min
  • 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

@mppalves mppalves added enhancement New feature or request Minor Smaller modifications High risk Higher risk labels Jan 30, 2022
config/default.cfg Show resolved Hide resolved
CHANGELOG.md Outdated

### added
- **tc** added new set 'tautype' spliting tau between croplands and managed pastures; New input file f13_pastr_tau_hist.csv
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not very clear to me (only by seeing the changelog) the role of the .csv file (also for the parameters and .cs3 in land). You could instead add a brief description of it? Something like: new input file which contains historical tc for pasture.

@@ -49,7 +49,8 @@ cfg$input <- c(regional = "rev4.67_h12_magpie.tgz",
#below it will get merged into cfg$repositories

cfg$repositories <- append(list("https://rse.pik-potsdam.de/data/magpie/public"=NULL),
getOption("magpie_repos"))
getOption("magpie_repos"))
# list("C:/magpie_inputdata/output"=NULL))
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented line is maybe coming from a merge?

main.gms Outdated Show resolved Hide resolved
pc13_tcguess(h) Guess for annual tc rates in the next time step (1)
i13_tc_factor(t) Regression factor (USD05PPP per ha)
i13_tc_exponent(t) Regression exponent (1)
pc13_land(i, tautype) Crop land area per region (mio ha)
Copy link
Contributor

Choose a reason for hiding this comment

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

Description change, Now it includes pasture as well

modules/10_land/feb15/declarations.gms Show resolved Hide resolved
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.

Great to see this development moving!
Just for clarification. With this PR you add data sets and dimensions for managed pasture. But these additions are not yet used, right?
For instance, pasture is not yet split and pasture yields are not yet changed. Is that correct?
If I'm not mistaken vm_tau(i,"pastr") is currently a free variable.

@mppalves
Copy link
Contributor Author

mppalves commented Jan 31, 2022

Great to see this development moving! Just for clarification. With this PR you add data sets and dimensions for managed pasture. But these additions are not yet used, right? For instance, pasture is not yet split and pasture yields are not yet changed. Is that correct? If I'm not mistaken vm_tau(i,"pastr") is currently a free variable.

Hi Florian! Yes, I am splitting the whole implementation into chunks. This is the first chunk of code, so vm_tau(i, "past") is still not engaged in any other module. It will be once all the other parts are put together! So, for this PR no significant changes in model behaviors are expected.

@@ -16,7 +16,7 @@
+ vm_cost_inv(i2)
+ sum((cell(i2,j2),land), vm_cost_landcon(j2,land))
+ sum((cell(i2,j2),k), vm_cost_transp(j2,k))
+ vm_tech_cost(i2)
+ sum(tautype, vm_tech_cost(i2,tautype))
Copy link
Member

Choose a reason for hiding this comment

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

don't do that. You will introduce various compatibility issues by changing the declaration of an interface. You can instead sum up the costs within the tc module and leave the vm_tech_cost interface untouched

v13_cost_tc(i) Technical change costs per region (mio. USD05PPP)
vm_tau(h,tautype) Agricultural land use intensity tau (1)
vm_tech_cost(i,tautype) Annuitized costs of TC (mio. USD05PPP per yr)
v13_cost_tc(i,tautype) Technical change costs per region (mio. USD05PPP)
;
Copy link
Member

Choose a reason for hiding this comment

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

your implementation is completely different to what is there so far. Hence, you need to create a new realization and do your changes in this new realization.

Copy link
Member

Choose a reason for hiding this comment

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

and try to reduce your changes in the interfaces! Changing interfaces will introduce a lot of work, e.g. a lot of changes in the magpie4 package and so on

@@ -9,6 +9,7 @@
parameters
pm_land_start(j,land) Land initialization area (mio. ha)
pcm_land(j,land) Land area in previous time step (mio. ha)
pcm_grass(j,grassland) Grassland areas in previous time step (mio. ha)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that you need that additional interface...in the tc module you could use pcm_land("past") instead even so it has a slightly different meaning

@mppalves mppalves closed this Mar 4, 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 High risk Higher risk Minor Smaller modifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants