Skip to content
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

restore modifications for correct computation of AC hydro profiles #1293

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

danielelerede-oet
Copy link
Contributor

Changes proposed in this Pull Request

Hi @davide-f , we noticed with @yerbol-akhmetov that after merging the content of PR #1120 there were issues with renewable profiles. Indeed, that contribution was removing some of the changes introduced by PR #1119 . I restored the missing ones so that the computation of hydro profiles when using alternative clustering should be working fine again.

Checklist

  • I consent to the release of this PR's code under the AGPLv3 license and non-code contributions under CC0-1.0 and CC-BY-4.0.
  • I tested my contribution locally and it seems to work fine.
  • [] Code and workflow changes are sufficiently documented.
  • [] Newly introduced dependencies are added to envs/environment.yaml and doc/requirements.txt.
  • Changes in configuration options are added in all of config.default.yaml and config.tutorial.yaml.
  • Add a test config or line additions to test/ (note tests are changing the config.tutorial.yaml)
  • Changes in configuration options are also documented in doc/configtables/*.csv and line references are adjusted in doc/configuration.rst and doc/tutorial.rst.
  • A note for the release notes doc/release_notes.rst is amended in the format of previous release notes, including reference to the requested PR.

Sorry, something went wrong.

@davide-f
Copy link
Member

Thanks for reporting @danielelerede-oet , I'll prioritize this.
Is it possible to be tested on NG? Have you tested it?
The issue of the previous implementation is that hydro inflows were allocated by bus, not bus region.
However, in alternative_clustering it is done by bus region [in the end] and the risk is double counting.
Have you tested this PR and satisfies your needs with/without alternative clustering?

Unfortunate that it was not tracked during the testing of the alternative_clustering before... really need to change this architecture after the wave has passed

@danielelerede-oet
Copy link
Contributor Author

Hi @davide-f , yes, I tested it on Nigeria, and compared with Voronoi clustering and previous results I already got and all works fine. We also need to test it on the US again after the fixes on the alternative clustering issue, but these modifications I performed should at least bring us back to the situation where hydro inflows were not overestimated. Just to clarify, nothing different is done with respect to #1119.

@davide-f
Copy link
Member

davide-f commented Jan 13, 2025

Update; by testing on tutorial (BJ+NG), these are results:
image
base is pypsa-earth main, running the tutorial, daniele_tutorial is this PR using tutorial and daniele_alternative is the tutorial case with the flag alternative_clustering true.

It seems on the good path, but few adjustments needed; I'll debug

@davide-f
Copy link
Member

davide-f commented Jan 13, 2025

Update

to improve validation, I've checked this PR (code name daniele), current main (upstream), current main before the recent PRs about alternative clustering (commit ef525ee) and the main with PR #1119 (commit 8073162), with/without alternative clustering on the tutorial case limited to Nigeria.

Note: expected value of total energy inflow in tutorial (that adopts hydro_capacities) with no spillover is:
(0.823336 # file hydro_capacity, column InflowHourlyAvg[GWh]
* 8760 # number of hours in a year
* 1.1 # multiplier in config file: renewable->hydro->multiplier
) = 7934 GWh (7.93 e+6 MWh)

Statistics of energy dispatch (focus hydro - storageunit):

image

Statistics of inflows

total inflows, calculated as the brutal sum of storage_units_t.inflow.sum().sum():
image

Comment

Overall the PR looks quite needed.
The total inflows highlighted an inconsistency of total inflow with/without alternative clustering.
The current PR (a) fixes the overestimation of inflow in alternative clustering, (b) ensures the tutorial and non-tutorial case to match but at a relatively lower value than in past analyses.
Results do not fully restore past behavior. Honestly, I've tested too many configurations across commits. Need better crosscheck tomorrow.

A possible issue during alternative_clustering is that:

  • with alternative clustering we have 1 onshore region for each gadm shape, and that shape is allocated to a random bus
  • hydro units, however, are not necessarily allocated to that specific bus, thus there is an inconsistency. The fix is that

This PR looks promising, and we can likely finalize soon.
The only concern to check is the points above. In this PR, alternative clustering, the total dispatch decreases with respect to before.
That may be acceptable but there is the hidden risk of losing some units because of the temporary solution of adopting 1 region in alternative clustering

For reproducibility:

config.yaml:

countries: ["NG"]

run:
  name: "ef525ee_tutorial" # use this to keep track of runs with different settings

cluster_options:
  alternative_clustering: false # "False" use Voronoi shapes, "True" use GADM shapes

with snakemake (code name _alternative) means alternative_clustering: true

snakemake command: snakemake solve_all_networks -call --configfile config.tutorial.yaml

Notebook attached for reproducibility if anyone is interested
comp.zip

@danielelerede-oet
Copy link
Contributor Author

Hi @davide-f , I get the same results as you in the (expectedly obvious but always better to check), and I think it's worth to notice that the lower value we got for dispatch is actually closer to historical generation in 2013 (5.273 TWh, as from eia_hydro_annual_generation). The situation is even "better" when using the eia normalization method:
image

@davide-f
Copy link
Member

Are you testing the version with eia? if yes good.
Generally hydro is spilled only as last resource, as such it should match the corresponding statistics.

Under further checking, unfortunately this PR risks to implicitly drop some hydro units because of the missing onshore regions of the buses the units are connected to.

This image shows the total inflows by hydro unit of the elec network. in this implementation, unfortunately 3 units are missing because of that
image
This can have implications in large countries with multiple units.

Probably the best approach is to revise the build_renewable_profiles implementation with hydro and change the current implementation that relies on the regions shapefile for the calculation towards the use of the lat/lon location of the hydro powerplant.
Should we have a dedicated PR for that or do you plan to intervene here?

For info, we would like to also merge the linopy PR soon [best tomorrow]
I would love to give priority to fixing the whole hydro stuff but probably the issue requires some more effort that I don't have immediately [tomorrow/thursday], so probably we could probably merge the linopy one first unless there are particular constraints

@ekatef ekatef mentioned this pull request Jan 14, 2025
@yerbol-akhmetov
Copy link
Collaborator

yerbol-akhmetov commented Jan 15, 2025

Hi, @danielelerede-oet, @davide-f. I have performed several runs for US with Voronoi and GADM clustering with EIA normalization for hydro using this PR. I have noticed also at the final network that capacities of hydro is very small for GADM clustering. As a result, generation for hydro was almost none. Here is the capacities in MW:
image

Here is the generation mix in TWh:
image

yerbol-akhmetov and others added 2 commits January 17, 2025 21:22

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Reassign buses for hydro powerplants
Copy link
Member

@davide-f davide-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @yerbol-akhmetov and @danielelerede-oet :D

Great work! I have a minor restyling to avoid a double if clause :) This PR should restore the previous behavior and we can merge soon if needed fast.
Just to be sure, have you verified that the alternative approach matches the voronoi one like in previous comments?

A more long-term solution could be to avoid relying on regions onshore for hydro, but we can also keep it for another moment. If we agree on keeping the long-term solution for another moment, then I'd add an issue for that and merge this.

What do you think?

Comment on lines +473 to +492
if snakemake.params.alternative_clustering:
# bus to region mapping for hydro powerplants
bus_to_region_id = (
ppl.drop_duplicates(subset="bus").set_index("bus")["region_id"].to_dict()
)

ppl = (
ppl.query('carrier == "hydro"')
.reset_index(drop=True)
.rename(index=lambda s: str(s) + " hydro")
)

with xr.open_dataarray(snakemake.input.profile_hydro) as inflow:
if snakemake.params.alternative_clustering:
for bus in inflow.indexes["plant"]:
if bus in bus_to_region_id:
region_to_update = bus_to_region_id[bus]
# Update all rows in the DataFrame where region_id matches
ppl.loc[ppl["region_id"] == region_to_update, "bus"] = bus

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if snakemake.params.alternative_clustering:
# bus to region mapping for hydro powerplants
bus_to_region_id = (
ppl.drop_duplicates(subset="bus").set_index("bus")["region_id"].to_dict()
)
ppl = (
ppl.query('carrier == "hydro"')
.reset_index(drop=True)
.rename(index=lambda s: str(s) + " hydro")
)
with xr.open_dataarray(snakemake.input.profile_hydro) as inflow:
if snakemake.params.alternative_clustering:
for bus in inflow.indexes["plant"]:
if bus in bus_to_region_id:
region_to_update = bus_to_region_id[bus]
# Update all rows in the DataFrame where region_id matches
ppl.loc[ppl["region_id"] == region_to_update, "bus"] = bus
ppl = (
ppl.query('carrier == "hydro"')
.reset_index(drop=True)
.rename(index=lambda s: str(s) + " hydro")
)
if snakemake.params.alternative_clustering:
# bus to region mapping for hydro powerplants
bus_to_region_id = (
ppl.drop_duplicates(subset="bus").set_index("bus")["region_id"].to_dict()
)
with xr.open_dataarray(snakemake.input.profile_hydro) as inflow:
for bus in inflow.indexes["plant"]:
if bus in bus_to_region_id:
region_to_update = bus_to_region_id[bus]
# Update all rows in the DataFrame where region_id matches
ppl.loc[ppl["region_id"] == region_to_update, "bus"] = bus

Just a minor restyling; could you try this? [we can avoid 1 if clause]

@yerbol-akhmetov
Copy link
Collaborator

yerbol-akhmetov commented Jan 19, 2025

Hello @yerbol-akhmetov and @danielelerede-oet :D

Great work! I have a minor restyling to avoid a double if clause :) This PR should restore the previous behavior and we can merge soon if needed fast. Just to be sure, have you verified that the alternative approach matches the voronoi one like in previous comments?

A more long-term solution could be to avoid relying on regions onshore for hydro, but we can also keep it for another moment. If we agree on keeping the long-term solution for another moment, then I'd add an issue for that and merge this.

What do you think?

Thank you for the comment! It would be great to refine it. I will make these changes.

Update. Hi, @danielelerede-oet. Here is the PR of address restyling.

@davide-f
Copy link
Member

davide-f commented Jan 21, 2025

Hello @yerbol-akhmetov and @danielelerede-oet :D
If you wish, to avoid the additional merges and if daniele is currently busy, yerbol may open another PR linking his branch and the commits will be integrated.
What do you think?

@yerbol-akhmetov to clarify the image above of the dispatch comparison, what is the row of the hydro?
Is it this one?
image

@yerbol-akhmetov
Copy link
Collaborator

yerbol-akhmetov commented Jan 21, 2025

Hello @yerbol-akhmetov and @danielelerede-oet :D If you wish, to avoid the additional merges and if daniele is currently busy, yerbol may open another PR linking his branch and the commits will be integrated. What do you think?

@yerbol-akhmetov to clarify the image above of the dispatch comparison, what is the row of the hydro? Is it this one? image

Hi, @davide-f. Thanks for the comment. Yes, I will open a separate PR then (update: link to PR). And sorry about the image without proper names. Here is the hydro:
image

Here is another results for generation. GADM EIA PR after is after my changes. Still hydro is missing. It is because I have taken region_id to buses mapping from powerplants.csv. But let me test it again with latest commits.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants