Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Revise network simplification to account for DC lines #743
Changes from 25 commits
44ee30d
64cf5d0
874e777
cb20186
e3a1ca5
a525321
97fa953
cd8b430
9847ca1
b7c2534
5082e04
baa817b
4a21588
18e63ed
9abb586
906588f
bd2bb13
a0a8a44
ebce63b
3827fb9
7cc768f
8125f8e
a6576f7
6864268
4b7ada4
5c377b1
0f727bf
de4a2bb
4d017cc
505213b
50b558f
08ac4e3
6af3be2
e44cdb1
29dc7d6
81678ef
8bed8c4
b1a11bd
e3c87f7
121bf2d
0b9f7a8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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
inn.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 withnum_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