-
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
Security analysis: support of load and generator contingencies #431
Conversation
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
@@ -21,6 +25,10 @@ | |||
|
|||
boolean hasVoltageControl(); |
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 wonder if hasVoltageControl
and hasReactivePowerControl
(indeed it means remote control here) are still useful because it could be replaced by gettint the generator control type.
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 is redundant indeed, but sometimes convenient
lfBus.setLoadTargetP(lfBus.getLoadTargetP() - load.getP0() / PerUnit.SB); | ||
lfBus.getLfLoads().setAbsVariableLoadTargetP(lfBus.getLfLoads().getAbsVariableLoadTargetP() - load.getP0()); | ||
for (Triple<String, Double, Double> loadInfo : contingency.getLoadIdsToLose()) { | ||
if (loadInfo.getLeft() != null) { |
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.
We have an issue here, I am going to fix it in next commits.
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
…/powsybl-open-loadflow into support-contingency-type
@@ -90,28 +91,39 @@ public PropagatedContingency(Contingency contingency, int index, Set<String> bra | |||
branchIdsToOpen.add(terminal.getConnectable().getId()); | |||
} | |||
if (terminal.getConnectable() instanceof Generator) { |
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 am not sure that the following lines are relevant and useful. Can you explain me why you introduce this loop on terminals to disconnect and if my code is relevant when we loose a load, a generator or a shunt ? Thanks a lot !
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.
Well, I didn't introduce that loop, but these lines are probably quite relevant and useful as they fill up the lists of disconnected equipments with the equipments traversed during propagation. I think those lists are then useful in the analysis - at least they are used a lot. But I feel like I misunderstood your question?
About the code correctness, I don't really get what you want me to check? indeed, you didn't really change that loop
(about authoring those lines, git blames me, but the loop has been introduced by #135)
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 few things after trying to understand this code though:
- I don't get why you store the targetP of the generator? it seems that it's not used in the analysis, you only use the id
- The Triple object you store for loads makes the code unclear, why didn't you introduce a specific object instead?
- Do we need to store parameters of disconnected shuntCompensators with this Pair list? whereas it looks like the LfShuntCompensator already holds that info
- and strangely
getShuntIdsToLoose
is not called in the SensitivityAnalysis, unlike all other lists, so there's maybe something missing? Not current PR's topic though.
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.
It is just the unit tests never go into that lines except the first if statement. So... I was wondering if it was relevant as it was introduced by non graph experts!
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.
- Do we need to store parameters of disconnected shuntCompensators with this Pair list? whereas it looks like the LfShuntCompensator already holds that info
Shunts are aggregated by buses in the LF network
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.
- and strangely
getShuntIdsToLoose
is not called in the SensitivityAnalysis, unlike all other lists, so there's maybe something missing? Not current PR's topic though.
Shunt contingencies don't work at ALL in security and sensitivity analysis
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 Triple object you store for loads makes the code unclear, why didn't you introduce a specific object instead?
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.
- I don't get why you store the targetP of the generator? it seems that it's not used in the analysis, you only use the id
Right, removed
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
# Conflicts: # src/main/java/com/powsybl/openloadflow/util/sa/PropagatedContingency.java
Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@gmail.com>
Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@gmail.com>
for (String loadId : contingency.getLoadIdsToLose()) { | ||
Load load = network.getLoad(loadId); | ||
Bus bus = load.getTerminal().getBusView().getBus(); | ||
if (bus != null) { |
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 reason for null checking here? If load is part of the calculation, it is connected to a bus?
this.shuntIdsToShift = Objects.requireNonNull(shuntIdsToShift); | ||
|
||
// WTF????? | ||
for (Switch sw : switchesToOpen) { |
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.
We will do it later, we there is something to fix here...
|
||
Map<LfShunt, Double> shunts = new HashMap<>(1); | ||
for (var e : shuntIdsToShift.entrySet()) { | ||
// FIXME does not work when multiple shunts connected to a bus because IDs won't be found !!!! |
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 was not the purpose of this PR, but shunt contingency during SA cannot work as we do not search with the right ID since we have aggregated shunts by buses with a derivated ID. To fix later?
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 don't understand the problem. For each IIDM shunt id (shunt.getIds()
gives a list of IIDM shunt ids) we can get the aggregated shunt. So for me if we loose 2 shunts connected to the same bus, the same aggregated LfShunt will be shift twice. The issue is maybe to use a HashMap or not to check if the LfShunt is already in the map ? Because indeed we have the same issue with load shift, no ?
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.
Right, I removed my comment
case TWO_WINDINGS_TRANSFORMER: | ||
// branch check is done inside tripping | ||
ContingencyTripping.createBranchTripping(network, element.getId()) | ||
.traverse(switchesToOpen, terminalsToDisconnect); |
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 removed here the adding to branchIdsToOpen, because it seems to me redundant with the post propagation processing as terminalsToDisconnect contains to initial starting equipment. Seems ok to you?
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.
Indeed I really don't know because it was my question to Florian but it tells me that it is not from him.
// dangling line check is done inside tripping | ||
ContingencyTripping.createDanglingLineTripping(network, element.getId()) | ||
.traverse(switchesToOpen, terminalsToDisconnect); | ||
break; |
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.
Same
|
||
private final Map<String, Double> generatorIdsToLose; | ||
|
||
private final Map<String, PowerShift> loadIdsToShift; |
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 used shift because is only part of an aggregate
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, better.
|
||
private final Map<String, PowerShift> loadIdsToShift; | ||
|
||
private final Map<String, Double> shuntIdsToShift; |
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 used shift because is only part of an aggregate
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, better.
} | ||
buses.forEach(b -> branches.addAll(b.getBranches())); | ||
|
||
for (String hvdcId : hvdcIdsToOpen) { |
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 added an exception to warm that HVDC line contingency is not yet implemented in SA and sense analysis
Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@gmail.com>
Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@rte-france.com>
Signed-off-by: Anne Tilloy <anne.tilloy@rte-france.com>
…/powsybl-open-loadflow into support-contingency-type
Kudos, SonarCloud Quality Gate passed! |
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 restYes, #241 .
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
For the moment, we only support branch and shunt contingencies in security analysis. With this PR, we will support also load and generator contingencies. I think that I will update the AC sensitivity analysis after this PR.
What is the current behavior? (You can also link to an open issue here)
Nothing.
What is the new behavior (if this is a feature change)?
It works except for a load contingency in a distributed slack on conform load. The extension load detail is not taken into account at this stage. The parameter that ensure power factor to be constant is not taken into account also.
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)