Feature non-electric vehicles in transport #394
Conversation
for more information, see https://pre-commit.ci
…ellot/euro-calliope into feature-transport-demand
There was a problem hiding this comment.
Thanks a lot for this. This looks like a substantial effort. It does require some refactoring which I've explained in my comments.
On the high-level, there are two main comments I have: First, updates to the documentation are missing. Second, this PR removes full-electrification which seems unnecessary to me. We had it implemented and shouldn't remove it as it's a valid modelling choice.
| @@ -34,7 +34,7 @@ wildcard_constraints: | |||
|
|
|||
| ruleorder: area_to_capacity_limits > hydro_capacities > biofuels > nuclear_regional_capacity > dummy_tech_locations_template | |||
| ruleorder: bio_techs_and_locations_template > techs_and_locations_template | |||
| ruleorder: create_controlled_road_transport_annual_demand_and_installed_capacities > dummy_tech_locations_template | |||
| ruleorder: create_road_transport_vehicle_parameters > create_transport_demand_data_for_yaml > dummy_tech_locations_template | |||
There was a problem hiding this comment.
Slightly unrelated to this PR but this looks like a code smell. "ruleorder" by itself is to be avoided where possible and this seems like it will continue to grow.
Couldn't we flip this and instead define techs and tech_groups requiring the "dummy_tech:locations_template" using a wildcard_constraint on that rule? That would allow to get rid of these ruleorder directives. Also, it would make the purpose of the "dummy_tech_locations_template" rule clearer -- I again forgot and don't fully understand what it is used for.
👆 This is a comment for @brynpickering , I guess.
| @@ -174,8 +175,7 @@ rule model_template: | |||
| ), | |||
| demand_timeseries_data = ( | |||
| "build/models/{resolution}/timeseries/demand/electricity.csv", | |||
| "build/models/{resolution}/timeseries/demand/uncontrolled-electrified-road-transport.csv", | |||
There was a problem hiding this comment.
I guess it'll get clearer somewhere further down in this PR, but I hope this doesn't mean you've removed the full-electrification option. That should remain imo.
There was a problem hiding this comment.
Ok it actually seems you've removed full-electrification. I don't think we should do that. Full-electrification is a useful feature and it can exist next to flexible electrification, right?
| @@ -333,19 +333,19 @@ properties: | |||
| additionalProperties: false | |||
| properties: | |||
| light-duty-vehicles: | |||
| type: number | |||
| type: object | |||
There was a problem hiding this comment.
I am sorry to say, but these should all be clearer defined. They need the "properties" property which then defines the "diesel" and "electricity" property as numbers. As these are repeating, you could define them as a reusable "definition". Have a look at the "feedstock" definition in line 3 for reference.
| parent: demand | ||
| carrier: light_transport | ||
| constraints: | ||
| force_resource: false |
There was a problem hiding this comment.
This is the default value and setting it is optional. I think we should be consistent: either we add this to all demands, or we remove it here, too.
| {% for idx in locations.index %} | ||
| {% for tech in locations.columns %} | ||
| {{ tech }}_annual_distance_{{ idx }}: | ||
| techs: [demand_{{ tech[5:] }}] |
There was a problem hiding this comment.
tech[5:] is hard to understand. Why only from the fifth element forward? Can we maybe remove [0:5] before handing it to the template, so we can avoid this selection? If not, a comment would be necessary.
| df, weights, left_index=True, right_index=True, suffixes=("_df", "_weights") | ||
| ) | ||
| for col in df.columns: | ||
| merged_df[col] = merged_df[col + "_df"] * merged_df[col + "_weights"] |
There was a problem hiding this comment.
Lines 144--148 seem unnecessarily complicated. You are basically multiplying df with weights. Why does this need to be more complicated than df.mul(weights)?
Is it maybe because you don't need all columns? Even then, a simple df[cols].mul(weights[cols]) should do the trick.
There was a problem hiding this comment.
Is it maybe because you need all three: df, weights, and df * weights?
If yes, this would be easier:
weighted = df.mul(weights)
merged = pd.merge(df, weights, left_index=True, right_index=True, suffixes=("_df", "_weights"))
merged = pd.merge(merged, weighted, left_index=True, right_index=True)| .apply(_get_efficiency, axis=1, args=(carrier,)) | ||
| .droplevel(0) | ||
| .swaplevel(0, 1) | ||
| .pipe( # Add aggregated vehicle_type column |
There was a problem hiding this comment.
As above, the pipe is not necessary.
| efficiency = _aggregate(efficiency, vehicle_type_aggregation) | ||
|
|
||
| # Missing years are assumed to be the same as the last available year | ||
| if final_year > 2015: |
There was a problem hiding this comment.
Just to be 100% sure, better check assert first_year <= final_year somewhere. Otherwise weird things could happen.
| import pycountry | ||
|
|
||
|
|
||
| def scale_to_regional_resolution(df, region_country_mapping, populations): |
| df_annual_road_distance_demand.index.name = "id" | ||
|
|
||
| # Multiply by transport unit and export to CSV | ||
| df_annual_road_distance_demand.mul(snakemake.params.transport_factor).to_csv( |
There was a problem hiding this comment.
As mentioned, better don't apply scaling factors here. Instead, add the default unit to the doc here:
https://euro-calliope.readthedocs.io/en/stable/model/overview/#units-of-quantities
Fixes #273, #274, #395 and #351 .
This PR introduces diesel vehicles on top of EVs, to reproduce the sector-coupled euro-calliope workflow.
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.