pypsa-meets-earth / pypsa-earth-sec

GNU General Public License v3.0
18 stars 16 forks source link

Consider "urban_percentage" based on "panning_horizons" wildcard and its effect on all Rules #338

Open Eddy-JV opened 2 weeks ago

Eddy-JV commented 2 weeks ago

Closes # (if applicable).

Changes proposed in this Pull Request

@energyLS PLEASE review this PR carefully. I added a wildcard for the urban_percentage year in Rule build_population_layouts and therefore added {planning_horizons} wildcard to all Rules after that.

This PR is part of adding Params to all Rules in Snakemake under Issue https://github.com/pypsa-meets-earth/pypsa-earth-sec/issues/330

The following Rules are already done including the work done in this PR:

Checklist

energyLS commented 2 weeks ago

@Eddy-JV thanks for the PR. Only concern is regarding the wildcard; I think for tracability and transparency we should move that to a seperate PR? Also, I think you have added the wildcard in order to work with myopic foresight? Can you elaborate on this? At the current stage I am hesitating to merge right away, since the new wildcard is a major change for all users.

Eddy-JV commented 2 weeks ago

@Eddy-JV thanks for the PR. Only concern is regarding the wildcard; I think for tracability and transparency we should move that to a seperate PR? Also, I think you have added the wildcard in order to work with myopic foresight? Can you elaborate on this? At the current stage I am hesitating to merge right away, since the new wildcard is a major change for all users.

I changed the name of the PR to be only for the urban_perecentage changes. I will do another PR to deal with the Params.

Eddy-JV commented 2 weeks ago

@energyLS Regarding why I introduced this. In the current workflow, urban_percentage for planning_horitons[0] is always used. Which is let's say 2030. But if we introduce the wildcard in Rule build_population_layouts and some of the following ones whenever necessary, we will be calculating all these changed rules using the wildcard year and not the first value.

energyLS commented 3 days ago

Looks great. Currently testing locally as well, if that works out we could merge. @Eddy-JV are there any concerns, or could we merge (given it works locally for me)?

@Eddy-JV has agreed bilaterally. Merging now.