-
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
Shunt compensator voltage control #391
Conversation
} | ||
} else { | ||
Optional<DiscreteVoltageControl> dvc = bus.getDiscreteVoltageControl(); | ||
if (dvc.isPresent() && dvc.get().getControllerBuses().contains(bus)) { |
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.
This part is the most hard to understand and probably the implementation is not the good one. In case of several shunts connected on the same bus, instead of creating several B variables, I have decided to create just one variable B that takes all we need to control voltage and to redispatch the amount of B on the physical shunts after the resolution.
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.
You have to use only one b variable, but maybe you could improve your implementation by spreading the b into each shunt by just adding a scalar to the term.
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.
Can you explain with more details what you have in mind ? In my implementation, I use only one b
variable on a bus, but if I have several shunts, I have to find the best combination of the sections of the shunts that is close to the solved b
. How can we do that through a scalar ?
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.
So we cannot just have a proportional distribution of b across all shunts directly in the equation system?
I don't remember why we create as many LfShunt by bus as in the IIDM network. Maybe it would be simpler to have only one LfShunt by LfBus that would aggregate IIDM shunt compensator. In the case of non regulating shunt compensators that would only required in the updateState method to distribute the q proportionally to individual b and in your regulating shunt case that would be simpler to model.
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.
The distribution could not be proportional to be optimal because each shunt have sections (in a different number too), that could be non linear too. So I have to fill the shunt one by one to find the optimal combinaison that is the closest to the continuous value. But for sure, it would be easier to have a single LfShunt by a bus, with one or two shunt equations: one for the constant B shunts and the other one for the regulating ones. But it is a huge change... and maybe the refactoring should be done before or after this PR.
} | ||
} else { | ||
Optional<DiscreteVoltageControl> dvc = bus.getDiscreteVoltageControl(); | ||
if (dvc.isPresent() && dvc.get().getControllerBuses().contains(bus)) { |
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.
You have to use only one b variable, but maybe you could improve your implementation by spreading the b into each shunt by just adding a scalar to the term.
@@ -17,15 +17,18 @@ | |||
public class DiscreteVoltageControl { | |||
|
|||
public enum Mode { | |||
VOLTAGE, | |||
VOLTAGE_TRANSFORMER, | |||
VOLTAGE_SHUNT, |
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.
What was the motivation to share the same object control with transformer one?
This look to me 2 different things.
The implementation looks now very confusing to me as we have 2 modes and 2 controller lists and that both have to be consistent.
What about renaming existing DiscreteVoltageControl
to TransformerVoltageControl
and adding a new ShuntCompensatorVoltageControl
?
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.
Okay, no problem to do that.
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.
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.
@flo-dup I think that we can simplify the code now by removing the notion of "discrete" for phase controls.
@@ -162,6 +162,7 @@ public static AcLoadFlowParameters createAcParameters(Network network, MatrixFac | |||
LOGGER.info("Connected component mode: {}", parameters.getConnectedComponentMode()); | |||
LOGGER.info("Voltage per reactive power control: {}", parametersExt.isVoltagePerReactivePowerControl()); | |||
LOGGER.info("Reactive Power Remote control: {}", parametersExt.hasReactivePowerRemoteControl()); | |||
LOGGER.info("Shunt voltage control: {}", parameters.isSimulShunt()); |
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.
A comment for the loadflow api: "simulShunt" is poorly named...
For a user point of view, what does "simulate shunts" means? It does not appear clearly that we are talking about shunt voltage control.
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. I have created an issue : powsybl/powsybl-core#1929
@geofjamg With your last merged PRs, it is my third huge rebase for that PR: I guess that it is my Christmas present! |
I will do it |
No that is okay, indeed I have time to do it and it will be a good way to understand your recent changes. |
src/main/java/com/powsybl/openloadflow/ac/ShuntVoltageControlOuterLoop.java
Show resolved
Hide resolved
|
||
for (LfBus controllerBus : vc.getControllers()) { | ||
// round the rho shift to the closest tap | ||
List<LfShunt> controllerShunts = controllerBus.getControllerShunts(); |
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.
I am obliged to OFF the VC after this dispatching.
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
7138367
to
270cddf
Compare
ShuntCompensatorReactiveFlowEquationTerm q = new ShuntCompensatorReactiveFlowEquationTerm(shunt, controllerBus, variableSet, false); | ||
terms.add(q); | ||
}); | ||
controllerBus.getControllerShunt().ifPresent(shunt -> { |
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.
I am not totally sure of this term.
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@gmail.com>
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.
- Maybe there is room for simplying the
LfShuntImpl
code - I suspect the shared remote control is not working. Like for any other shared control you need to add extra equation for b distribution => as there is no real use case and as it will complexity the code, I would not support it now but throws an exception instead and only support local and non shared remote control.
- My main concern is that there is exactly the same functional issue as for transformer voltage control. There is no management of min/max susceptance. Like for transformers at a given loop, if some shunt reach the min or max b, they should be switched to fixed value (min ou max value) and the other ones should continue to control voltage at next loop. Everything stop when no other shunt reach min or max or if all shunts have reached min or max b. Also control mode should not be managed at control object level but at each of the controller bus so that control mode status is the result of individual controller status. This will allow for a given control to have at some moment, part of the controllers controlling and other ones not (if at min/max). But maybe we should let it like this and fix both transformer and shunt voltage control afterward in another PR.
@@ -56,12 +59,20 @@ private double v() { | |||
return stateVector.get(vVar.getRow()); | |||
} | |||
|
|||
private double b() { | |||
return bVar != null ? stateVector.get(bVar.getRow()) : shunt.getB(); // FIXME |
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.
Is the FIXME still relevant?
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.
No, I am going to remove it, thanks!
@@ -22,7 +22,7 @@ public void onDiscretePhaseControlModeChange(DiscretePhaseControl phaseControl, | |||
} | |||
|
|||
@Override | |||
public void onDiscreteVoltageControlModeChange(DiscreteVoltageControl voltageControl, DiscreteVoltageControl.Mode oldMode, DiscreteVoltageControl.Mode newMode) { | |||
public void onDiscreteVoltageControlModeChange(DiscreteVoltageControl voltageControl, DiscreteVoltageControl.Mode newMode) { |
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.
Is it on purpose that you removed the oldMode?
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.
Just because it is not used in the code.
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.
I would keep it as it can be useful later
|
||
private double zb; | ||
|
||
private static class ControllerLfShunt { |
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.
Not clear to me why you need to create this intermediate data model
public LfShuntImpl(List<ShuntCompensator> shuntCompensators, LfNetwork network) { | ||
super(network); | ||
if (shuntCompensators.isEmpty()) { | ||
throw new IllegalArgumentException("Empty shunt compensator list"); | ||
} | ||
this.shuntCompensators = Objects.requireNonNull(shuntCompensators); | ||
for (ShuntCompensator sc : shuntCompensators) { |
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.
But looking at how you build ShuntImpl, there either only shunts controlling voltage or only shunts not controlling voltage?
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, so I think that you are right I can simply the LfShuntImpl
knowing that.
I am going to try to have only one list of ShuntCompensator but it am not sure I can do better without loosing in readability.
No it is not supported, I have to commit my work: it is "easy" to support it as we do for transformers but it has to be refactor later.
Yes, I know because you explain me that for transfromers. But I really prefer that we merge this PR first and deal with that later (just after this one). The rebases are really really time consuming for me. For the control mode, I understand that you want me to add a |
If you can, try to do it using just activation and not equation removal. |
ok |
Fix linear model support. Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Please retry analysis of this Pull-Request directly on SonarCloud. |
:-) |
Please check if the PR fulfills these requirements (please use
'[x]'
to check the checkboxes, or submit the PR and then click the checkboxes)**Does this PR already have an issue describing the problem ? If so, link to this issue using '#XXX' and skip the rest
Yes, an issue: #107
**What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
For the moment, this PR is a first minimal feature that supports:
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:
Other information:
(if any of the questions/checkboxes don't apply, please delete them entirely)