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.

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

Target vector update refactoring #262

Merged
merged 7 commits into from
Apr 16, 2021
Merged

Target vector update refactoring #262

merged 7 commits into from
Apr 16, 2021

Conversation

geofjamg
Copy link
Member

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

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change or deprecate an API? If yes, check the following:

  • The Breaking Change or Deprecated label has been added
  • The migration guide has been updated in the github wiki (What changes might users need to make in their application due to this PR?)

Other information:

(if any of the questions/checkboxes don't apply, please delete them entirely)

Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@rte-france.com>
Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@rte-france.com>
Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@rte-france.com>
@geofjamg geofjamg changed the title Target vector Target vector update refactoring Mar 30, 2021
@geofjamg geofjamg requested review from flo-dup and annetill March 30, 2021 21:21
Copy link
Contributor

@flo-dup flo-dup left a comment

Choose a reason for hiding this comment

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

Nice exending the use of network listener. Just a couple of comments

}

public double[] toArray() {
if (status == Status.VALID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just being meticulous, but as you're always returning this.array, I'd maybe add an updateArray method, and call createArray in case of a vector invalid.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, done.

@@ -102,7 +102,7 @@ public DcLoadFlowResult run(Reporter reporter) {

equationSystem.updateEquations(x);

this.targetVector = equationSystem.createTargetVector();
this.targetVector = TargetVector.createArray(network, equationSystem);
Copy link
Contributor

@flo-dup flo-dup Apr 16, 2021

Choose a reason for hiding this comment

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

Shouldn't we do something similar for DC, later maybe? Indeed, I think that the target vector update could be used for sensitivity analyses, where we could avoid to create the vector several times. Not sure the gain is interesting though.

Copy link
Member Author

@geofjamg geofjamg Apr 16, 2021

Choose a reason for hiding this comment

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

Maybe it could be interesting but not as much as for AC power flow because for DC power we have just either one or two iteration depending on stack distribution activated or not. For AC power we have often a lot of iterations and number of target vector updates, that is why it is in this case very interesting to automate the update. So yes let's see that later.

@@ -283,7 +297,14 @@ public double getLoadTargetQ() {

@Override
public void setLoadTargetQ(double loadTargetQ) {
this.loadTargetQ = loadTargetQ * PerUnit.SB;
double newLoadTargetQ = loadTargetQ * PerUnit.SB;
if (newLoadTargetQ != this.loadTargetQ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I realized this old/new test is only there because of restoring all bus states instead of only those which have changed, maybe I should have a look there if there's something to do. and if it's worth it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed we could probably check if restoration is relevant on BusState/BranchState side but in any case I think it is safer to add this guard here.

Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@rte-france.com>
@sonarqubecloud
Copy link

Kudos, SonarCloud 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

@geofjamg geofjamg merged commit 2dd2420 into master Apr 16, 2021
@geofjamg geofjamg deleted the target_vector branch April 16, 2021 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants