Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix dispatchQ method #529

Merged
merged 2 commits into from
May 5, 2022
Merged

fix dispatchQ method #529

merged 2 commits into from
May 5, 2022

Conversation

EtienneLt
Copy link
Contributor

Signed-off-by: Etienne LESOT etienne.lesot@rte-france.com

Please check if the PR fulfills these requirements (please use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Does this PR already have an issue describing the problem ? If so, link to this issue using '#XXX' and skip the rest

#526

@AnkurArohi AnkurArohi assigned AnkurArohi and unassigned AnkurArohi May 4, 2022
@AnkurArohi AnkurArohi closed this May 4, 2022
@AnkurArohi
Copy link

Sorry by mistake it was closed

@AnkurArohi AnkurArohi reopened this May 4, 2022
@AnkurArohi
Copy link

I do not have the option to merge the request but I see all good Thanks.

@EtienneLt EtienneLt closed this May 4, 2022
@EtienneLt EtienneLt reopened this May 4, 2022
@EtienneLt
Copy link
Contributor Author

I do not have the option to merge the request but I see all good Thanks.

Thank you for the review !

@EtienneLt EtienneLt self-assigned this May 4, 2022
@AnkurArohi
Copy link

Aha, I still have a point but it is probably seldom this case lets say even after many iterations the tobedispatchedq is not below the tolerance limit
what do we do now?

@EtienneLt
Copy link
Contributor Author

EtienneLt commented May 4, 2022 •
edited
Loading

Aha, I still have a point but it is probably seldom this case lets say even after many iterations the tobedispatchedq is not below the tolerance limit what do we do now?

I think all the generators will be at the max or min q bound so the loop stops (the generator list will be empty).
But you're right when this happens there should be at least a log

@AnkurArohi
Copy link

But then additional to the log the amount of tobedispatchedq should be then dispatched to the slack all of it ,naturally we need another Load Flow to confirm that the network is secured and stable but ideally at the end there should be no tobedispatchedQ left

sylvlecl
sylvlecl reviewed May 5, 2022
View reviewed changes
Copy link
Contributor

@sylvlecl sylvlecl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try to improve the "integration test": it passed before the fix

src/test/java/com/powsybl/openloadflow/ac/AcLoadFlowEurostagTutorialExample1Test.java Show resolved Hide resolved
src/test/java/com/powsybl/openloadflow/ac/AcLoadFlowEurostagTutorialExample1Test.java Show resolved Hide resolved
@EtienneLt
fix dispatchQ method
52d5c33 
Signed-off-by: Etienne LESOT <etienne.lesot@rte-france.com>
@AnkurArohi
Copy link

@EtienneLt
The suggestion for the case of distributed slacks is an intresting one
should i create a new issue or how do we handle this

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@EtienneLt
Copy link
Contributor Author

@EtienneLt The suggestion for the case of distributed slacks is an intresting one should i create a new issue or how do we handle this

It seems that it is already implemented

@sylvlecl sylvlecl self-requested a review May 5, 2022 13:28
@sylvlecl sylvlecl merged commit 0e2d23c into main May 5, 2022
@sylvlecl sylvlecl deleted the fix-dispatch-q branch May 5, 2022 13:41
@AnkurArohi
Copy link

🏁🏁🏁🏁🏁😉

annetill
annetill reviewed May 6, 2022
View reviewed changes
src/main/java/com/powsybl/openloadflow/network/impl/AbstractLfBus.java
double residueQ = 0;
double calculatedQ = qToDispatch / generatorsThatControlVoltage.size();
if (generatorsThatControlVoltage.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is not needed because it is done just before calling this method.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my suggestion and was to ensure that through debug log User is aeare that there has indeed been no redistribution

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus we should always consider

Separation of concerns based implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnkurArohi AnkurArohi AnkurArohi left review comments

@annetill annetill annetill left review comments

@sylvlecl sylvlecl sylvlecl approved these changes

Assignees

@EtienneLt EtienneLt

Labels
In Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@EtienneLt @AnkurArohi @sylvlecl @annetill