Feature: heat pump cop profiles #393
Conversation
|
I am confused. Why are you intending to merge into the |
To reduce the diff that you would be reviewing. Sorry for pushing this before the hot water one was ready! I thought it would be nice to have it online so that you could review it while I was fixing up review comments on the hot water PR. |
scripts/heat/heat_pump_cop.py
Outdated
| assert ( | ||
| cop_summer > cop_winter | ||
| ).all(), "Found higher heating COP values in winter than in summer." | ||
| assert (cop >= 1).all(), "Found improbably low heat pump COP values (< 1)." |
There was a problem hiding this comment.
These would make great unit tests. I would move them there.
There was a problem hiding this comment.
pros of moving to unit tests:
- more clear grouping of our tests
- availability of pytest infrastructure
cons:
- you don't find out something is a problem until the entire workflow has been completed (should we instead be aiming for fast failure...?)
- the test is far removed from the code that would cause test failure
- (in this example) the test would come after the data aggregation. I'd fix this by moving the aggregation of gridcells to IDs into the next rule in the process, so this rule's outputs can be fed into a test, but then the output files are quite large.
There was a problem hiding this comment.
I guess this is a more high level question of when to test in a snakemake workflow. I'm have implemented the change locally but do wonder whether the cons outweigh the pros...
There was a problem hiding this comment.
I see your point and I agree this may require a high-level discussion. I just want to add another important pro: you can test many more things and, because it's pytest, quite easily. For example, you can execute each test by country.
There was a problem hiding this comment.
In terms of code modularity, it would be good to have tests (especially unit tests) closer to the actual scripts. In theory we could have a test runner per "module" (== ".smk" file), but it would add overhead.
There was a problem hiding this comment.
Moved although I maintain that for intermediate datasets it is better to do the checks as part of the script. For final model files, we can then have tests. This stops us having an overcomplicated DAG where loads of intermediate data files need to be kept around until the penultimate step.
Do you mean extending to having the electricity demand from heating be based on the when2heat profile & heat pump COP? Or do you mean having heat pumps as a templated technology option? |
|
Either of the two, but it seems the latter makes more sense. It wouldn't be a big effort, would it? It's just so that this PR is not just some unconnected data processing step that'll make sense only after other PRs are merged but a feature that can be integrated and used right away. |
|
@timtroendle I have:
It was a bit more work than expected ;) I also found it a pain to debug tests as you can't drop into the debugger with |
|
Failing tests are due to too many requests to access either the miniconda |
| @@ -188,30 +189,39 @@ parameters: | |||
| nuts-year: 2013 | |||
| heat: | |||
| space_heat: | |||
| carnot-performance: 0.36 # [Nouvel_2015] | |||
There was a problem hiding this comment.
Minor cleanup here to remove some unused configs and update refs (still more to do)
| type: number | ||
| description: Heat pump coefficient of performance. | ||
| water_heat: | ||
| hot_water: |
There was a problem hiding this comment.
water_heat -> hot_water applied everywhere for consistency
|
|
||
| # Weather data is subset by the geographic area covered by model run (given by available population sites) | ||
| temperature_ds = xr.open_dataset(path_to_temperature).sel(site=population.site) | ||
| wind_ds = xr.open_dataset(path_to_wind_speed).sel(site=population.site) | ||
| temperature_ds = xr.open_dataset(path_to_temperature) |
There was a problem hiding this comment.
No longer doing pre-selection of sites so that this step is resolution agnostic
|
|
||
| grouped_hourly_heat = xr.merge([grouped_hourly_space, grouped_hourly_water]) | ||
| grouped_hourly_heat.to_netcdf(out_path) | ||
| grouped_hourly_heat = xr.merge([ |
There was a problem hiding this comment.
Moved sanity checks to tests
There was a problem hiding this comment.
Awesome!
A few more minor changes. More importantly: updates to the docs are changing. There should be updates here:
https://euro-calliope.readthedocs.io/en/latest/model/customisation/#importing-modules
In the sake of modularity, shouldn't we split the heat yaml template into several files? I tend to think yes. Also, I would add additional techs in additional PRs, together with the respective carriers they require.
tests/model/test_heat.py
Outdated
| @@ -0,0 +1,25 @@ | |||
| def test_cop_monthly(cop_timeseries): | |||
There was a problem hiding this comment.
I guess you could do that per location. Would that be useful?
There was a problem hiding this comment.
It's effectively being done per location anyway, right? If the test fials, you'd be able to pinpoint the offending location pretty quickly when debugging...
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| gridded_data = xr.open_dataset(snakemake.input.gridded_timeseries_data) |
There was a problem hiding this comment.
First use of this approach in the repository. The line will make history. :D
| Args: | ||
| gridded_data (xr.Dataset): Gridded timeseries space heat and hot water data. | ||
| grid_weight (xr.DataArray): Weighted mapping from grid (a.k.a. "site") to units. | ||
| annual_demand (xr.DataArray): Per-unit annual space heating and hot water demands. |
There was a problem hiding this comment.
I find it hard to understand why this input is necessary. Can you explain? Is this a bias-correction? Or is it because the time series have no meaningful scale / are normed?
There was a problem hiding this comment.
It's doing two sets of weighted averages: weighted averaging "sites" into "IDs" (using population) and weighted averaging "space heat" and "hot water" demand into a general profile for "heat" (using annual demands).
| pd.DataFrame: Index: timeseries, Columns: unit IDs | ||
| """ | ||
|
|
||
| partial_func = partial( |
There was a problem hiding this comment.
maybe rename function to "apply_weights" or similar?
| monetary: | ||
| # TODO: weight averaging per region based on dwelling ratio | ||
| energy_cap: {{ mean([5.9 / 10, 5.9 / 8, 76 / 400, 45 / 160]) * 1e6 * scaling_factors.specific_costs }} # {{ (1 / scaling_factors.specific_costs) | unit("EUR2015/MW_heat") }} | average of new/existing and single-/multi-family homes. | ||
| om_annual: {{ mean([0.42 / 10, 0.42 / 10, 1.343 / 400, 0.889 / 160]) * 1e6 * scaling_factors.specific_costs }} # {{ (1 / scaling_factors.specific_costs) | unit("EUR2015/MW_heat/year") }} | average of new/existing and single-/multi-family homes. |
| monetary: | ||
| # TODO: weight averaging per region based on dwelling ratio | ||
| energy_cap: {{ (mean([7.6 / 10, 5 / 4, 114 / 400, 57 / 160]) * heat_pump_shares.ashp + mean([12 / 10, 9 / 4, 202 / 400, 72 / 160]) * heat_pump_shares.gshp) * 1e6 * scaling_factors.specific_costs }} # {{ (1 / scaling_factors.specific_costs) | unit("EUR2015/MW_heat") }} | average of new/existing and single-/multi-family homes | weighted by assumed share of installed GSHP vs ASHP. | ||
| om_annual: {{ (mean([0.222 / 10, 0.222 / 4, 0.761 / 400, 0.761 / 160]) * heat_pump_shares.ashp + mean([0.222 / 10, 0.222 / 4, 0.761 / 400, 0.761 / 160]) * heat_pump_shares.gshp) * 1e6 * scaling_factors.specific_costs }} # {{ (1 / scaling_factors.specific_costs) | unit("EUR2015/MW_heat/year") }} | average of new/existing and single-/multi-family homes | weighted by assumed share of installed GSHP vs ASHP. |
There was a problem hiding this comment.
All three comments from above apply here too.
| name: Heat pump | ||
| parent: conversion | ||
| carrier_in: electricity | ||
| carrier_out: hp_heat |
There was a problem hiding this comment.
What's this carrier necessary for? For the different temperature level?
There was a problem hiding this comment.
For the storage buffer to be specific to that technology
| lifetime: 30 | ||
| costs: | ||
| monetary: | ||
| # TODO: weight averaging per region based on dwelling ratio |
There was a problem hiding this comment.
And again all three comments apply.
There was a problem hiding this comment.
In terms of modularity, should this file include only heat pumps? I tend to think yes. Also, I'd prefer to include other supply technologies in other PRs, together with their carrier supplies which seem missing in this PR.
There was a problem hiding this comment.
oops, sorry. A staging error. I'll revert the non-electricity ones. I'd maybe do it per type of carrier in?
|
I wouldn't worry too much about the failing tests. @setup-miniconda's tests are failing, too, so I am assuming they will have this high up on their agenda. |
|
👆Oops, I wanted to suggest edits and now I've committed them. Must have missed the "suggest" option, sorry. |
* Split out heat tech templates * Remove boilers from heat techs * Add locations to heat tech template * Update DEA references * Improve commenting for heat tech cost calcs * Rearrange gridded timeseries script for clarity
|
@timtroendle I think that's all done now. I've opened various issues for things that need to be resolved off the back of this PR.
|
There was a problem hiding this comment.
Nice!
Just one, I think important, comment: there is a dependency between the "storage/heat" and "supply/heat" files now which should not be the case. By moving the overrides to the "storage/heat" you should be able to get rid of the dependency. The only remaining dependency are the carrier definitions and we will not and don't have to get rid of those.
| electric_heater: | ||
| {% endfor %} | ||
|
|
||
| overrides: |
There was a problem hiding this comment.
These overrides should be in the templates/models/techs/storage/heat.yaml, right? Otherwise we are creating a dependency between these two files.
I mean, this will break if users don't import the file with the storage techs.
There was a problem hiding this comment.
Also: consider adding these overrides to tests/resources/test.yaml.
There was a problem hiding this comment.
We're creating a dependency either way as they also influence the output carrier of the supply technology. I think they are better off in supply/heat-from-electricity.yaml as the storage techs are actually abstract tech groups so there's nothing wrong with including that YAML file as an input by default.
As an aside: this won't be an issue in v0.7 as we can trigger a storage buffer for these kinds of techs when defining them, so I wouldn't put too much weight on whether we've got it perfect right now.
There was a problem hiding this comment.
Ok up to you. I guess you have the better overview.
I just think we should try to minimise dependencies between files as much as possible. As far as I can tell, this would be the first dependency between two files. It's also slightly surprising that storage gets added through a file in "supply" instead of "storage".
There was a problem hiding this comment.
OK, I've decoupled them now, although this does add a bit of unnecessary model complexity when not using the storage buffers. Since this will all be swept away with v0.7 that complexity is probably fine for now.
…-heat-pump-cop-profiles Feature: heat pump cop profiles
…-heat-pump-cop-profiles Feature: heat pump cop profiles
…-heat-pump-cop-profiles Feature: heat pump cop profiles
Fixes #80 by adding COP profiles.
Builds onchanges made to input weather data in #390.
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.