Refactor graph traversal methods to use new computeTraversalPartitions API#3753
Refactor graph traversal methods to use new computeTraversalPartitions API#3753clementleclercRTE wants to merge 28 commits intomainfrom
computeTraversalPartitions API#3753Conversation
…API. Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
…API. Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
… graph-connected-components # Conflicts: # iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/NodeBreakerTopologyModel.java
math/src/main/java/com/powsybl/math/graph/UndirectedGraphImpl.java
Outdated
Show resolved
Hide resolved
math/src/main/java/com/powsybl/math/graph/UndirectedGraphImpl.java
Outdated
Show resolved
Hide resolved
getConnectedComponents …getConnectedComponents API
math/src/main/java/com/powsybl/math/graph/UndirectedGraphImpl.java
Outdated
Show resolved
Hide resolved
… traversal components. Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
…Predicate` Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
…` API, and enhance vertex object handling in graph traversal implementations. Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
|
Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
… graph-connected-components
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/BusBreakerTopologyModel.java
Outdated
Show resolved
Hide resolved
math/src/main/java/com/powsybl/math/graph/UndirectedGraphImpl.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/BusBreakerTopologyModel.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/BusBreakerTopologyModel.java
Outdated
Show resolved
Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/BusBreakerTopologyModel.java
Outdated
Show resolved
Hide resolved
| } | ||
| return TraverseResult.CONTINUE; | ||
| }).forEach(nodes -> { | ||
| CopyOnWriteArrayList<NodeTerminal> terminals = new CopyOnWriteArrayList<>(); |
There was a problem hiding this comment.
I'm wondering if we really need the CopyOnWriteArrayList for buses
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/NodeBreakerTopologyModel.java
Outdated
Show resolved
Hide resolved
math/src/main/java/com/powsybl/math/graph/UndirectedGraphImpl.java
Outdated
Show resolved
Hide resolved
…comments Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
…comments Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
… graph-connected-components
Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
flo-dup
left a comment
There was a problem hiding this comment.
The naming is questionable, as we're not computing connected component if we rely on graph theory definition. We should rather call it something like computeTraversalPartitions, as we're partitioning the graph (all the vertices are traversed if there's no TERMINATE_TRAVERSER return) based on a given traversal.
math/src/main/java/com/powsybl/math/graph/UndirectedGraphImpl.java
Outdated
Show resolved
Hide resolved
| C component = collector.createComponent(); | ||
| collector.addVertex(component, v); | ||
|
|
||
| traverse(v, TraversalType.BREADTH_FIRST, (v1, e, v2) -> { |
There was a problem hiding this comment.
If traverse returns false, shouldn't we break?
There was a problem hiding this comment.
Fixed. Note that TERMINATE_TRAVERSER should not normally be used here.
Using TERMINATE_TRAVERSER would result in an incomplete partition. The break is just a safety net.
There was a problem hiding this comment.
Your note should be included in the javadoc of computeTraversalPartitions
… graph-connected-components
… update related tests, topology models, and traversal logic. Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
getConnectedComponents APIcomputeTraversalPartitions API
|
|
||
| private void traverseVertex(int vToTraverse, boolean[] vEncountered, Deque<DirectedEdge> edgesToTraverse, | ||
| TIntArrayList[] adjacencyList, TraversalType traversalType) { | ||
| TIntArrayList[] adjacencyList, TraversalType traversalType, @Nullable IntConsumer vertexVisitor) { |
There was a problem hiding this comment.
we usually don't need nullability annotations on private methods. Besides, a nullable functional interface is creating unnecessary complexity (my bad, I suggested you to do so in a previous comment!), you'd rather use a no-op IntConsumer instead of null
There was a problem hiding this comment.
Remove the use of the deprecated traverse in tests
| if (sw != null && sw.isOpen()) { | ||
| return TraverseResult.TERMINATE_PATH; | ||
| } | ||
| return TraverseResult.CONTINUE; |
There was a problem hiding this comment.
An edge cannot be null in DcTopologyModel
| if (sw != null && sw.isOpen()) { | |
| return TraverseResult.TERMINATE_PATH; | |
| } | |
| return TraverseResult.CONTINUE; | |
| return sw.isOpen() ? TraverseResult.TERMINATE_PATH : TraverseResult.CONTINUE; |
| } | ||
|
|
||
| private boolean traverse(int v, TraversalType traversalType, Traverser traverser, boolean[] encounteredVertices, boolean[] encounteredEdges) { | ||
| return traverse(v, traversalType, traverser, encounteredVertices, encounteredEdges, null); |
There was a problem hiding this comment.
| return traverse(v, traversalType, traverser, encounteredVertices, encounteredEdges, null); | |
| return traverse(v, traversalType, traverser, encounteredVertices, encounteredEdges, vIndex -> {}); |
|
|
||
| void addVertex(C component, int vertexIndex); | ||
| } | ||
| <C> List<C> computeTraversalPartitions(Traverser traverser, Supplier<C> partitionFactory, ObjIntConsumer<C> vertexConsumer); |
There was a problem hiding this comment.
You need to add javadoc to explain clearly the method and the parameters.
Besides, rename vertexConsumer into something more explicit (vertexCollector?)
|
Could you also complete the PR description, regarding the migration guide / breaking changes part? |
… graph-connected-components
Signed-off-by: Leclerc Clement <clement.leclerc@rte-france.com>
|



Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
What kind of change does this PR introduce?
Refactoring / New API
What is the current behavior?
Currently, BusBreakerTopologyModel, NodeBreakerTopologyModel, and DcTopologyModel all contain duplicated logic to traverse the graph and identify connected components (bus construction).
What is the new behavior (if this is a feature change)?
A new method
computeTraversalPartitionshas been introduced in theUndirectedGraphinterface. It centralizes the graph traversal and connected component building logic, using standardfunctional interfaces (
Supplier,ObjIntConsumer) for flexibility. The three topology models have been refactored to use this shared method.The previously public method
traverse(int, TraversalType, Traverser, boolean[])is now deprecated in favor oftraverse(int, TraversalType, Traverser)andcomputeTraversalPartitions(...).Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
1. Deprecated:
traverse(int, TraversalType, Traverser, boolean[])If you were using this overload to manually track visited vertices, switch to the standard
traverse(int, TraversalType, Traverser)overload. Visited vertices can be captured directly inside yourTraverserlambda.2. Manual connected component computation
If you were manually implementing the traversal-and-partition pattern (iterating over vertices, tracking visited state, grouping results), you can now replace this logic with a call to
computeTraversalPartitions(Traverser)or its generic overloadcomputeTraversalPartitions(Traverser, Supplier<C>, ObjIntConsumer<C>).Other information: