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 islanded buses and add clustering by networks - clean branch #632
Fix islanded buses and add clustering by networks - clean branch #632
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.
Very nice PR :)
temporary comments in the meantime :)
Let us know when ready
Thanks a lot! :) Wouldn't it be more convenient to move back to #614? I have been experimenting a bit with git history and #614 has been cleaned-up as well. Although, checking results are better here :) Let me know please your preferences Currently there are some problems with reproducibility of the workflow. Not sure what exactly is the reason |
Mmmm... simplified networkclustered network(with minimum 28 buses) Note please that the each of the seemingly isolated buses in |
Update: the above are effects of some specific config settings. The workflow and topology model seems to be quite sensitive to it. Probably, some kind of visual guide would be helpful to understand the model behaviour better. By proper config setting, Testing results for Central Asia look quite appropriate: kmeans (by load)hac (by load)hac (by gdp)gadmWhat is really surprising, hac algorithm doesn't require to decrease a number of clusters to a very few in this config setup. Not sure it's good news as I was not able to understand which exactly config changes had such an effect |
for more information, see https://pre-commit.ci
@davide-f, I think this PR is ready. Testing on CD as well as CD + ZM was successful for all clustering types. I think we may assume that the introduced changes work properly. However, there are some pitfalls along the workflow which can make difficult to reproduce clustering results. In particular, I'd keep in sight using pypsa-earth/scripts/simplify_network.py Lines 442 to 451 in 6f01921
It can lead to the errors which are difficult to catch as idxmin() being applied to the row of inf gives a pretty arbitrary result. Being overlayed with variations in output of clustering algorithms and the overall complexity of our workflow, it can give really fascinating consequences :D |
Sorry for messing-up the branch history :( Haven't meant it, honestly! |
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.
A little comment, please modify but this PR looks good to me :)
@pz-max do you want to take a look or shall we go?
This is a quite heavy PR.
scripts/simplify_network.py
Outdated
p_threshold_drop_isolated = max( | ||
0.0, cluster_config.get("p_threshold_drop_isolated", 0.0) | ||
) | ||
p_threshold_merge_isolated = cluster_config.get("p_threshold_merge_isolated", 0.0) |
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.
In this case, let's have False as adefault: that means this merge is not executed.
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.
Done
Merged, many thanks @ekatef ! |
Super :) Thank you so much, @davide-f! |
A cleaned version of #614
Checklist
envs/environment.yaml
andenvs/environment.docs.yaml
.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.