add affected terminals to traverse in TrippingTopologyTraverser to trace which terminals have been traversed #2040
Conversation
…ace which terminals have been traversed.
| assertEquals(switchIds, switchesToOpen.stream().map(Switch::getId).collect(Collectors.toSet())); | ||
| Set<Terminal> affectedTerminals = new HashSet<>(); | ||
| tripping.traverse(network, switchesToOpen, terminalsToDisconnect, affectedTerminals); | ||
| assertEquals(affectedTerminals.stream().map(Terminal::getConnectable).map(Connectable::getId).collect(Collectors.toSet()), |
There was a problem hiding this comment.
Why having removed this assertion ? :
assertEquals(switchIds, switchesToOpen.stream().map(Switch::getId).collect(Collectors.toSet()));
There was a problem hiding this comment.
should not have been removed, fixed
Signed-off-by: jacques borsenberger <jacques.borsenberger@rte-france.com>
| @@ -26,10 +26,10 @@ protected AbstractInjectionTripping(String id) { | |||
| } | |||
|
|
|||
| @Override | |||
| public void traverse(Network network, Set<Switch> switchesToOpen, Set<Terminal> terminalsToDisconnect) { | |||
| public void traverse(Network network, Set<Switch> switchesToOpen, Set<Terminal> terminalsToDisconnect, Set<Terminal> affectedTerminals) { | |||
There was a problem hiding this comment.
"affected" is not the correct word: traversedTerminal?
| @@ -41,6 +41,8 @@ public TraverseResult traverse(Terminal terminal, boolean connected) { | |||
| terminalsToDisconnect.add(terminal); | |||
| } | |||
| return TraverseResult.TERMINATE_PATH; | |||
| } else if (affectedTerminals != null) { | |||
| affectedTerminals.add(terminal); | |||
There was a problem hiding this comment.
Maybe we should only fill traversed terminal in case of bus/breaker topo. Even if in that case we will have the same list of terminal than in terminalsToDisconnect, but that would more consistent.
…dd_vertex_collection_to_find_all_path
…ttps://github.com/powsybl/powsybl-core into add_vertex_collection_to_find_all_path
There was a problem hiding this comment.
I don't see why we couldn't have reused terminalsToDisconnect instead of adding a new set traversedTerminals. And then simply adding it in the TopologyKind.NODE_BREAKER case. It seems to lead to the same result as what's done here. You probably already tried that, could you tell me what I'm missing?
I think the initial use of terminalsToDisconnect only for bus/breaker topo? |
But maybe you are right we can change its meaning to simplify the API but in that case, it is a serious breaking change. |
|
Yes it was only bus/breaker topology. Reusing it for node/breaker topology (and renaming it to traversedTerminals?) would do the same as what's done here... but then we change the behaviour of the traverser for that topology, so indeed a serious breaking change. |
|
But maybe it is not a so serious breaking change because people that we using this utillity class for a bus/breaker topo were not using terminalsToDisconnect, but just switches |
|
actually, after calling traverse, operation are called on each elements of terminalsToDisconnect this may change some behavior, so I'll need to add guard depending on network type, |
Thanks for pointing that out... Indeed that would lead to some extra care afterwards which makes it heavier to use. |
|
Kudos, SonarCloud Quality Gate passed! |
miovd
changed the title
…ace which terminals have been traversed.
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 restWhat 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:
Other information:
(if any of the questions/checkboxes don't apply, please delete them entirely)