-
Notifications
You must be signed in to change notification settings - Fork 1
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
Rertofitting review [DO NOT MERGE] #8
base: master
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
Fix gas generators
add clipping to solid_biomass_potentials
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: Martha Frysztacki <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
set OCGT capacities from generators to links
for more information, see https://pre-commit.ci
…ities Set existing capacities
for more information, see https://pre-commit.ci
Make links non-extendable if not configured in config'
This reverts commit 0bd66d3.
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.
@martacki Have added my comments to the code. Partially, they relate to implementation improvement. Also, I remember your concerns regarding p_min_pu/p_max_pu
, but agree that it looks like a rather minor issue.
Unfortunatelly, there are also a number of major points which make me think that the current implementation may give over-optimistic results.
- the heating demand is not modified, when calculating capital costs by different groups of residential buildings. That means that the capital costs for residential sector are underestimated.
Example: let's assume we have 100 MW of peak heating demand, with 50m^2 of AB by 10 euro/m2, 50m^2 of SFH by 100 euro/m2. Old implementation would give (10 * 50 + 100 * 50)/100 = 110/2 = 55 euro/MW for all building stock, while in a new implementation we have 1050/100 = 5 euro/MW and 50100/100 = 50 euro/MW. The average is 27.5 euro/MW which is two times lower as compared with the previous average value.
However, that is by no means an improvement, rather a result of failing to account that the heating demand is lower for a single category of buildings than for the whole building stock. Would we account that in fact each sector has only a share of the overall 100 MW, we would get 10 euro/MW and 100 euro/MW with the average of 55 euro/MW kept invariant.
It could make sense to validate the proposed approach comparing capital costs of the retrofit generators before and after modifications. The average costs by country must stay in the modified model the same as previously.
- I don't get the point in applying the weighting coefficients to the retrofitting costs in a way as it's implemented now. Not sure where do you suppose to apply the resulted weighting coefficients. I feel that is may be a risk of double-counting of costs differentiation between different groups of building, while the current implementation of the differentiation still leads to underestimation of retrofitting costs for the residential sector due to the reasons described in the point 1.
capital_cost = ( | ||
retro_data.loc[(ct, sec_i), ("cost")] | ||
* floor_area_node | ||
/ ((1 - dE) * space_heat_demand.max()) |
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.
As mentioned in Discord discussion, this chunk contains inconsistencies which mac potentially lead to a heavy under-estimation of capital costs for the residential sector.
The reasons are the following:
- space_heat_demand distinguishes only between residential and services sectors (according to this line, where sec variable may be only "tot", "residential" or "services");
- in contrast, floor_area_node differentiates between different buildings types, according to this line, where sec_i may be "residential AB", "residential MFH", along with "residential SFH", "residential" and "services".
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.
Hi @ekatef, thanks for the thorough review! I do agree with you that this part is not sufficienty tested and a preliminary version of what we could do.
However, as also mentioned on discord, the code we're using is not the version where sec_i may be "residential AB", "residential MFH" or "residential SFH", it is, as in the previous implementation, only either "residential" or "services"
The configuration to get to this point is setting (in the config.yaml file)
weighting_by_building_types = True
, and
disaggregate_building_types = False
What you're commenting on, is the setting
weighting_by_building_types = False
, and
disaggregate_building_types = True
which indeed is biased and underestimates costs / overestimates space heat demand savings for every of the building types. This part is only adapted so far in build_retro_costs.py
, but needs additional input in prepare_sector_network.py
(on exactly the points you mentioned), which we're pushing to the back in case there will be a follow up study (but not using for this study)
If you will, we can also include a logger.warning
not to use this setting.
building_data["sector"] = ( | ||
building_data.apply( | ||
lambda b: "residential SFH" if | ||
b.subsector == 'Single family- Terraced houses' else | ||
"residential AB" if b.subsector in "Appartment blocks" else | ||
"residential MFH" if b.subsector == "Multifamily houses" else | ||
"residential mixed" if b.subsector in ["Residential sector", ] else | ||
b.sector, axis=1) | ||
) |
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 part may be implemented in a cleaner way using a combination map()
with a mapping dictionary.
area_missing.sector = area_missing.apply( | ||
lambda b: | ||
"residential AB" if b.sector == "residential" | ||
else b.sector, axis=1 | ||
) |
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.
Apartment blocks are a not very good default, as it introduces a bias towards lower retrofitting costs. Using an average value would be a better solution.
if snakemake.params["retrofitting"]["disaggregate_building_types"]: | ||
u_values_PL.sector.replace({"residential": "residential AB"}, inplace=True) |
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.
As mentioned above, using average values is a better strategy to define defaults.
for component in components: | ||
for component 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 has been the reason for this replacement. To me, the implementation doesn't look self-explaining and may be difficult in maintainance.
residential_weighting = ( | ||
area_reordered | ||
.query("sector == 'residential'") | ||
.reset_index() | ||
.groupby(["country_code", "subsector"]) | ||
.sum() | ||
.value | ||
) | ||
services_weighting = ( | ||
area_reordered | ||
.query("sector != 'residential'") | ||
.reset_index() | ||
.groupby(["country_code", "subsector"]) | ||
.sum() | ||
.value | ||
) | ||
services_weighting = services_weighting.reset_index() | ||
services_weighting.subsector = "services" | ||
services_weighting = services_weighting.set_index( | ||
["country_code", "subsector"] | ||
).groupby(["country_code", "subsector"]).sum() | ||
|
||
weighting_by_building_type = pd.concat( | ||
[residential_weighting, services_weighting] | ||
).loc[cost_dE.index].reset_index() |
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 chunk look a bit awkward and is prone to errors due to duplications. It seems that the aim is to apply a custom aggregation function to the grouped dataframe. It that is a correct understanding, the implementation may be quite easily revised.
weighting_by_building_type["weight"] = weighting_by_building_type.apply( | ||
lambda b: b.value/weighting_by_building_type.query( | ||
"country_code == @b.country_code and subsector != 'services'" | ||
).value.sum() if b.subsector != 'services' else 1, | ||
axis=1 |
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.
If I get the idea properly, this part will leave retrofitting costs same for services, but will decrease them for the residential sector. Not sure what is physical reasoning behind this procedure.
As an additional comment, there is still weighting by for different categories of buildings earlier in the code, which accounts for the difference between the residential and service sectors:
pypsa-eur/scripts/build_retro_cost.py
Lines 215 to 224 in f26fca6
area_tot = area[["country", "sector", "value"]].groupby(["country", "sector"]).sum() | |
area = pd.concat( | |
[ | |
area, | |
area.apply( | |
lambda x: x.value / area_tot.value.loc[(x.country, x.sector)], axis=1 | |
).rename("weight"), | |
], | |
axis=1, | |
) |
Not sure what has been the reason re-implementing it instead of re-using with a desired aggregation function.
A technical pull request created to facilitate reviewing changes in retrofitting methodology.