-
Notifications
You must be signed in to change notification settings - Fork 184
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
Revise network simplification to account for DC lines #743
Conversation
Current implementation seems to reproduce desirable topology transformations, but should be adjusted in part of related to equivalent parameters calculation of the resulted links |
The suggested modification of before modificationLine,bus0,bus1
49423895_0,30,36
224147292_0,36,51
261165614_0,37,53
383097966_0,37,52
460419487_0,30,36
461638193_0,36,51
626214770_0,44,37
778351007_0,51,8
778351009_0,51,8
778351020_0,44,37
979570585_0,53,52
979570586_0,52,30
979570588_0,52,30
Link,bus0,bus1
convert_3_8,8,7
convert_29_44,44,43 after modificationLine,bus0,bus1
383097966_0,37,52
Link,bus0,bus1
261165614_0+1,52,37
460419487_0+4,52,7
778351020_0+1,43,37 The PR ensures that a result of the network simplification is the same regardsless of So, the next step is to resolve an issue with the duplication remained in the simplified network |
It looks like So, merging |
Hello @ekatef and very interesting contributions! Much needed. Many thanks! :) |
Hello @davide-f and many thanks for support, as usually! :) The numbers refer to the second picture in the comment above. I have been thinking that lines duplication left after simplification could be a bug which can be fixed by some simple trick. But it looks some deeper changes are needed if we are not happy with the resulted configuration of a simplified network. The question is if it's really needed :D |
As discussed with @davide-f, the best approach to tackle this issue with the parallel links remained after simplification might be just to apply |
It looks like we have previously missed some modifications in calculations on transmission costs needed to make representation of HVDC as lines work properly. In particular:
Have drafted possible implementation of these changes in @davide-f would be very grateful if you could find time to have a look :) |
Hello katia :) |
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.
Amazing job! :D
I've added some comments, on the chaining functionality in simplify_network I recall we talked about that and was ok.
I'll need another round maybe guided to address the functionality, but it was working well , right?
Fantastic Katia! :D
Thank you so much, Davide! There are still some troubles with functionality which I'm going to work on during the weekend. I'll ping you when ready :) |
Testing status: seems to work except a case when the augmentation option on:
So, augmentation needs better investigation while otherwise PR is ready for testing. @Tomkourou if you wish to help with testing, you are absolutely welcome :) |
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! I think this PR will be merged by next meeting , we are very close and great contribution! :)
So, is the algorithm finalized in your opinion? @ekatef
Is the only problem related to CD with augmentation by hvac?
scripts/simplify_network.py
Outdated
dc_as_links = not n.links.loc[n.links.carrier == "DC"].empty | ||
dc_as_lines = not n.lines.loc[n.lines.carrier == "DC"].empty |
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.
were you looking for the flag in config maybe? In case, you could pass it as a parameter here, what do you think?
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.
Yeah, I have been thinking about taking a flag directly from the config, but decided to try reading it from the network itself. The idea was to make it safer, although agree that it might be not clear.
Happy to revise if you think it'd be better to used a flag from the config
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.
Mah... agree that we could avoid overcomplicating the code.
We could do the check internally, however, it may be safer to check for dc lines:
dc_as_links = not (n.lines.carrier == "DC").any()
and implicitly assume that when it is false than we used lines
As even when lines are included, converters represented as links will be necessary as well, therefore, with the current formulation dc_as_links will always be true.
the if below where you check: if dc_as_links: .... elif dc_as_lines
, may be revised with: if dc_as_links: ... else: ...
so replacing the elif with else.
I'm unsure if some issues may be fixed with this but i'm unsure
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.
That is definitely cleaner :)
Thanks for explanations! Done
scripts/simplify_network.py
Outdated
p_nom = ( | ||
n.lines.loc[all_dc_lines] | ||
.apply( | ||
lambda x: x["v_nom"] * n.line_types.i_nom[x["type"]], | ||
axis=1, | ||
result_type="reduce", | ||
) | ||
.min() | ||
) |
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.
Theoretically, there should be the column s_nom that contains the nominal capacity of the line: is it filled properly?
If so, you may use that column here.
However, you raised an interesting point: in simplify_network_to_380
we still assume that all lines are AC, it may be interesting to debug what happens there with DC lines.
In particular, the num_parallel is most likely calculated in a wrong way
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.
Mmm, you are absolutely right: there is s_nom
in n.lines
, and it's value is exactly 3 times lower as compared with U*I used above. It feels like there is in fact some discrepancy with num_parallel
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.
Interesting check, so, the comment here may be that instead of calculating the power by using V*I , it may be better to directly use s_nom.
So regarding the code, I recommend to use s_nom here directly, ideally fixing the calculation of V*I for dc lines.
What do you think?
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.
Fixed
@davide-f thank you for the amazing review! Very heplful :D My feeling is that we are in fact quite close to finalise. From my perspective, augmentation is a major problem left. Apart of that, I'm also concerned with the recent testing results. All the tested countries work with base networksimplified networkclustered network |
Clustering results might be generally not very stable in respect to parameters selection like to remove stubs or not to remove :) But it would be a good idea to double-check that the clustering issues above are not being introduced with this PR. On the other hand, clustering problems were found for CN and IN only where old pre-calculated shapes were used. Probably, that has also played a role... @davide-f I do remember that you have suggested to account for generators/loads attached to the nodes when making simplification. At the moment the algorithm doesn't do for that, while my feeling is that it could be worth to improve this |
I confirm that clustering troubles are not connected with this PR. A test for IN with the same configuration parameters has resulted in the same limitation on |
scripts/simplify_network.py
Outdated
p_nom = ( | ||
n.lines.loc[all_dc_lines] | ||
.apply( | ||
lambda x: x["v_nom"] * n.line_types.i_nom[x["type"]], | ||
axis=1, | ||
result_type="reduce", | ||
) | ||
.min() | ||
) |
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.
Interesting check, so, the comment here may be that instead of calculating the power by using V*I , it may be better to directly use s_nom.
So regarding the code, I recommend to use s_nom here directly, ideally fixing the calculation of V*I for dc lines.
What do you think?
Great @ekatef :D merged :) |
Closes #621
Changes proposed in this Pull Request
DC lines are added to a simplification procedure to be treated in the same way as links.
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.NB Testing configuration assumed dc_as_lines for all the countries above and gadm clustesing for CD, PH and NG (as gadm clustering seems currently to be more prone to troubles) while usual hierarchical clustering was used for JP, IN and CN to use pre-calculated shapes keeping computational costs moderate.