-
Notifications
You must be signed in to change notification settings - Fork 8
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 SA exception "No slack bus for network" #1097
Conversation
if (slackBuses.isEmpty()) { // ultimate fallback | ||
selectedSlackBus = SLACK_BUS_SELECTOR_FALLBACK.select(busesByIndex, excludedSlackBuses.size() + maxSlackBusCount); | ||
selectedSlackBus = SLACK_BUS_SELECTOR_FALLBACK.select(selectableBus, excludedSlackBuses.size() + maxSlackBusCount); |
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.
Hello, a small remark here : There is no excluded slack buses in selectableBus
so the limit can be modified to just maxSlackBusCount
?
Is there a use case that can cover this line ? Maybe using a NameSlackBusSelector ? It seems that Sonar wants more coverage on this file :)
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.
Yes you are right thanks. I am thinking of an example to test fallback mechanism right now.
4095bd8
to
584a59d
Compare
@SylvestreSakti I added a test for the fallback mechanism. There is however a single line remaining uncovered and this is the exception when the fallback fail. |
@obrix Yes it seems hard to test this corner case. I agree with you that it can be merged as it is. To clarify : do committers have the right to merge against SonarCloud ? Or should it be done by the owner ? |
@SylvestreSakti I think once we agreed committer can do it, anyway we managed to cover it, thanks for the review! |
Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
Signed-off-by: Bertrand Rix <bertrand.rix@artelys.com>
1fb8859
to
18cc9d7
Compare
|
Please check if the PR fulfills these requirements
updateSlackBusesAndReferenceBus implements a slack bus selection algorithm using an exclude list of buses in excludedSlackBuses.
Some time a bus is selected because it is the one and only with the highest voltage. If this bus is also in the exclude list then we have no solution available.
I changed a bit the algorithm by filtering the bus list with the exclude list before applying the selection algorithm so it is not able to base its selection on buses that will not be available anyway.
To illustrate the bug surfaced on a small network similar to this :
B1(vl 150) ---- B2(vl 150) ---- B3 (vl380)
A contingency cut the line between B2 and B3. B3 is then put in the exclude listof selectable slack bus because it is no longer in the main connected component after contingency.
But B3 is also the one with the highest voltage level. So it was selected by the algorithm but ultimately discarded because of the exclude list.
Filtering the list of buses with the exclude list before the selection process fix the issue.
Does this PR already have an issue describing the problem?
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
IllegalStateException : "No slack bus for network " thrown
What is the new behavior (if this is a feature change)?
Correct slack bus is selected and no exception thrown
Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
Other information: