-
Notifications
You must be signed in to change notification settings - Fork 18
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
Feature ehighways resolution unts 318 #336
Feature ehighways resolution unts 318 #336
Conversation
for more information, see https://pre-commit.ci
…on files and schemas.
Now need to implement the actual function properly.
Debug configuration set up also.
…nathan-peel/euro-calliope into feature-ehighways-resolution-unts-318
for more information, see https://pre-commit.ci
References to nuts-year "variable" still exist.
…for all Europe (except Kosovo)
for more information, see https://pre-commit.ci
A a note: ehighways uses NUTS3 2006 (checked by comparing the region names in anderski_2014 with `statistal-to-ehighways-units.csv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! It looks good, but it requires a few changes, like type hints, documentation, and solving some inconsistencies.
The one thing I am not liking stems from the source: There are now two different ways in which resolutions are defined: the previously-existing shape
config in the config AND the new statistical-to-custom-units
file. Ideally, there would be only one place.
I don't think this should be handled within this PR or ticket. But I would like to keep this in mind and consider solving in the future. @brynpickering, do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many functions in this file lack type hints.
@@ -39,6 +39,9 @@ data-pre-processing: | |||
MNE: [HRV] | |||
SRB: [HUN] | |||
CYP: [ROU] | |||
statistical-to-custom-units: | |||
# FYI: E-highways were initially defined using NUTS3 units from 2006, but this mapping was manually created to map NUTS units from any year up to 2016 to E-highways. This is picked up by rule `units` to build custom spatial units. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comments seams out of context here. I don't know where this should go, but this looks like the wrong place to me.
@@ -314,6 +317,7 @@ scope: | |||
first-year: 2016 | |||
final-year: 2016 | |||
shapes: # This config must be consistent with data from https://doi.org/10.5281/zenodo.3244985. | |||
# TODO: update this link to Bryn's new one, which includes the potentials for ehighways |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the link
Norway: nuts3 | ||
Serbia: gadm0 | ||
Switzerland: nuts3 | ||
Iceland: nuts0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iceland is not currently supported by Euro-Calliope, see #15.
type: object | ||
description: >- | ||
Path to a csv file which defines how lower-level statistical units | ||
(i.e., NUTS and GADM) are combined to form units of a custom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GADM are not statistical units.
function. | ||
|
||
Inputs: | ||
- path_to_statistical_to_custom_units: csv file that specifies which NUTS and GADM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here again, there should be no statistical
in the name if you want to be more flexible than eHighways.
- layer: the GeoDataFrame covering the whole scope, where each country is formed | ||
from units corresponding to the resolution specified in resolution_config | ||
Outputs: | ||
- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Output is missing.
- | ||
|
||
Assumes: | ||
- the base-level statistical units in layer and are either NUTS3 or whole countries (NUTS0 or GADM0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GADM is not... you know what. :D
@@ -377,6 +394,10 @@ properties: | |||
type: string | |||
description: Nuclear technology capacity scenario. "current" will use today's capacities based on the JRC power plant database (PPDB). All other scenario names should point to a configuration CSV file specifying minimum and maximum capacities per country in MW with non-zero capacities (columns=[country, min, max]). | |||
pattern: ^(current)|(\w+.csv)$ | |||
nuts-year: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter does not exist in the config. Please remove.
""" | ||
|
||
stat_to_custom_units_df = pd.read_csv(path_to_statistical_to_custom_units, header=0) | ||
nuts_year = 2013 # This was a variable in Bryn's SC EC, but hardcoded to 2013 in EC. Consider that the year needs to match for both the (downloaded nuts units) and (nuts to custom region mapping) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a constant at the top of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove the first sentence. Alternatively, point to the line in the repo. "Bryn's SC EC" will not make sense to everyone and forever.
I don't think it's necessarily a problem. You have standardised boundaries coming from various data sources (nuts, gadm, ...) and they are your building blocks in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the issue is with setting a nuts year at the config level and then downloading that nuts year when generating any units (as I did in SC-EC). Since we only use NUTS for ehighways
at the moment it effectively allows you to choose the NUTS year that will be used to create those shapes.
We'll need the ability at a later point to download arbitrary NUTS years (for subnationalising using Eurostat datasets) so I guess we can leave it until then to introduce it as a feature.
# sum all of the units together into one block | ||
units = _continental_layer(units) | ||
elif path_to_statistical_to_custom_units != []: | ||
# this code is only executed if there is a [statistical-to-custom-units file corresponding to the chosen resolution] in the config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should get the same result with elif path_to_statistical_to_custom_units:
. An empty list resolves to False and a non-empty noe resolves to True in these kinds of conditional statements
Fixes #318 .
Checklist
Any checks which are not relevant to the PR can be pre-checked by the PR creator. All others should be checked by the reviewer. You can add extra checklist items here if required by the PR.