-
Notifications
You must be signed in to change notification settings - Fork 173
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 an error in simplify_network when to_substations simplification flag is on #708
Fix an error in simplify_network when to_substations simplification flag is on #708
Conversation
2ad4a37
to
bea3ebb
Compare
@davide-f, I think that is ready for the review |
As a comment, a reason for the issue was referring to |
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.
Nice PR and basically ready for merging after a simple comment :)
scripts/simplify_network.py
Outdated
n.buses.assign(bus_index=n.buses.index.get_level_values(0)) | ||
.loc[i_suffic_load] | ||
.groupby("country")["bus_index"] | ||
.first() | ||
.to_dict() |
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.
For consistency, I'd use "bus_id" instead of bus_index.
Is there a specific reason for this naming?
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.
Agree, revised
No rational reason, just a habit to keep the original data :)
Thank you! Fixed :) |
scripts/simplify_network.py
Outdated
@@ -658,7 +659,11 @@ def merge_isolated_nodes(n, threshold, aggregation_strategies=dict()): | |||
|
|||
# all the nodes to be merged should be mapped into a single node | |||
map_isolated_node_by_country = ( | |||
n.buses.loc[i_suffic_load].groupby("country")["bus_id"].first().to_dict() | |||
n.buses.assign(bus_id=n.buses.index.get_level_values(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.
Sorry a comment: why get_level_values here? isn't buses.index enough?
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.
Our data structure seem to be quite flexible, while multi-indexing is a usual pythonic practice. So, I thought it could be natural to use a general approach
However, looking into the code, it seems that we assume everywhere that n.buses always only one index. So, get_level_values(0)
looks like over-complication. Removed
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 comment
Have removed over-complication :) |
Super! Merging:) |
Closes #646
Changes proposed in this Pull Request
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.