-
Notifications
You must be signed in to change notification settings - Fork 207
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
Fix overlapping bus regions when alternative clustering is selected #1287
Fix overlapping bus regions when alternative clustering is selected #1287
Conversation
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 thanks for the PR :)
I have proposed two minor comments related. This is a great improvement! :)
Please add a bugfix in release_note :)
In your run, could you check if in the solved network hydro technologies are available?
An easy way is to do:
n = pypsa.Network("pypsa-earth/results/networks/...")
n.statistics()
There you can see if the hydro technologies do have a positive energy dispatch?
We had reports of potential issues and the reason is the following.
While for wind/solar technology the selection of the node is not so relevant, for hydro technology it is not because they are not as distributed as solar or wind.
Note, a possible fix for the hydro issue could be available and ready to review here:
#1249
#1249 is planned for review and probably the 2 PR together can solve the alternative_clustering issues :) [review and testing is needed]
scripts/build_bus_regions.py
Outdated
isolated_buses = n.buses[~non_isolated_buses].index | ||
# drop isolated buses | ||
onshore_regions = onshore_regions[~onshore_regions.name.isin(isolated_buses)] | ||
# drop duplicates based on shape_id | ||
onshore_regions = onshore_regions.drop_duplicates("shape_id") |
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.
isolated_buses = n.buses[~non_isolated_buses].index | |
# drop isolated buses | |
onshore_regions = onshore_regions[~onshore_regions.name.isin(isolated_buses)] | |
# drop duplicates based on shape_id | |
onshore_regions = onshore_regions.drop_duplicates("shape_id") | |
non_isolated_regions = onshore_regions[~onshore_regions.name.isin(isolated_buses)] | |
isolated_regions = onshore_regions[onshore_regions.name.isin(isolated_buses)] | |
# Combine regions while prioritizing non-isolated ones | |
onshore_regions = ( | |
pd.concat([non_isolated_regions, isolated_regions]) | |
.drop_duplicates("shape_id", keep="first") | |
) |
Could you check if this works? this also allows isolated nodes to be kept if necessary. It builds on your spirit.
The logic is that non_isolated_nodes have priority over isolated ones and isolated may be kept if no other bus is available there.
Could you check if it works?
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.
Thank you @davide-f. I checked and this works! I think it could add some stablility.
So this code prefers non-isolated bus but isolated one could represent a region if the region got only isolated ones.
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.
The edited code be like
if snakemake.params.alternative_clustering:
# determine isolated buses
n.determine_network_topology()
non_isolated_buses = n.buses.duplicated(subset=["sub_network"], keep=False)
isolated_buses = n.buses[~non_isolated_buses].index
non_isolated_regions = onshore_regions[~onshore_regions.name.isin(isolated_buses)]
isolated_regions = onshore_regions[onshore_regions.name.isin(isolated_buses)]
# Combine regions while prioritizing non-isolated ones
onshore_regions = (
pd.concat([non_isolated_regions, isolated_regions])
.drop_duplicates("shape_id", keep="first")
)
if len(onshore_regions) < len(gadm_country):
logger.warning(
f"The number of remaining of buses are less than the number of administrative clusters suggested!"
)
scripts/build_bus_regions.py
Outdated
logger.error( | ||
f"The number of remaining of buses are less than the number of administrative clusters suggested!" | ||
) | ||
raise ValueError("Insufficient buses to match administrative clusters!") |
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.
Do you mind making it a logger.warning instead?
I understand the reason but a hard error may have a hard impact
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.
Sure. I was not sure whether I need to put logger.warning or logger.error. Thanks.
As you said @davide-f, when I checked using modified I have to say #1249 and this PR should be used together. |
Hello @choiHenry :D |
5c92667
to
1eb3b34
Compare
for more information, see https://pre-commit.ci
@davide-f I pushed the commit! Thanks :) |
Closes # (if applicable).
Changes proposed in this Pull Request
Checklist
envs/environment.yaml
anddoc/requirements.txt
.config.default.yaml
andconfig.tutorial.yaml
.test/
(note tests are changing the config.tutorial.yaml)doc/configtables/*.csv
and line references are adjusted indoc/configuration.rst
anddoc/tutorial.rst
.doc/release_notes.rst
is amended in the format of previous release notes, including reference to the requested PR.