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.

Sign up for GitHub

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

Jump to bottom

Consecutive shunts #361

Merged
merged 19 commits into from
Mar 10, 2022
Merged

Consecutive shunts #361

merged 19 commits into from
Mar 10, 2022

Conversation

flo-dup
Copy link
Contributor

@flo-dup flo-dup commented Mar 7, 2022

Please check if the PR fulfills these requirements

  • 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 ?
No

What kind of change does this PR introduce?
Bug fix

What is the current behavior?
When a so called shunt node is detected in ImpliciCellDetector::detectAndTypeShunt, 3 cells are created:

  • one pure external cell with branches leading either to a bus or to a feeder
  • one shunt cell obtained by traversing the graph from that shunt node until a node with more than 2 neighbours is reached
  • one external cell with the remaining nodes

If there are several consecutive shunt cells in the diagram, this detection leads to an unhandled pattern in CellBlockDecomposer.

What is the new behavior (if this is a feature change)?
When a so called shunt node is detected in ImpliciCellDetector::detectAndTypeShunt, several cells are created:

  • one pure external cell with branches leading either to a bus or to a feeder
  • one or more shunt cells obtained by traversing the graph from that shunt node until a node with more than 2 neighbours is reached
  • in the remaining nodes, we're looking again for shunt nodes

Does this PR introduce a breaking change or deprecate an API?
No

@flo-dup flo-dup requested a review from BenoitJeanson March 7, 2022 09:44
flo-dup added 12 commits March 8, 2022 15:31
@flo-dup
delete CellBlockDecomposer::determineBlocks
7766beb 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Rename ShuntCell cells parameter to sideCells
9477ee8 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Add missing Override annotations
9f986ec 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Remove unnecessary casting
edb02b9 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Remove dead code
9392606 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Fix javadoc
b0b07e2 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Remove unnecessary internal nodes
284cb09 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Add test case
10c108f 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
An extern cell may have several shunt cells
ae12405 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Fix aligning shunt-connected cells directions
79b7f4e 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Update unit tests (same rendering)
37d836c 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Do not create cell right away if suspected shunt
4429f09 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup flo-dup force-pushed the consecutive_shunts branch from ae99f09 to 4429f09 Compare March 8, 2022 14:31
flo-dup added 5 commits March 8, 2022 15:42
@flo-dup
Fix code smell
9e03fdb 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Add comment
dfc7c1e 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Factorize code
35ae3a1 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Revert CRLF LF changes in reference file
a21acdc 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup
Merge branch 'main' into consecutive_shunts
43c0fe2 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@flo-dup flo-dup force-pushed the consecutive_shunts branch from 86e784e to 43c0fe2 Compare March 9, 2022 09:55
@flo-dup
Merge branch 'main' into consecutive_shunts
a801a88 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
BenoitJeanson
BenoitJeanson reviewed Mar 10, 2022
View reviewed changes
Copy link
Contributor

@BenoitJeanson BenoitJeanson left a comment

Choose a reason for hiding this comment

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

OK for me

single-line-diagram-core/src/main/java/com/powsybl/sld/layout/ImplicitCellDetector.java
graph.conditionalExtensionOfNodeConnectedToBus(node ->
node.getType() == Node.NodeType.SWITCH && ((SwitchNode) node).getKind() != SwitchNode.SwitchKind.DISCONNECTOR
|| node instanceof Middle2WTNode || node instanceof Middle3WTNode
);
graph.extendFirstOutsideNode();
Copy link
Contributor

Choose a reason for hiding this comment

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

It is required for some weird networks to have this fix... is this deleted line feature covered elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, indeed, and in fact it's required by some networks in unit tests. But I only moved that line one line below to have the conditional extension before, which allows to get rid of some spurious internal node like the one in the middle below:
screenshot

single-line-diagram-core/src/main/java/com/powsybl/sld/model/ExternCell.java Outdated
} else {
minHv = hPosition;
}
int minHv = shuntCells.stream().filter(shuntCell -> shuntCell.getSideCell(RIGHT) == this).findFirst()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why findFirst() only? Shouldn't we take the max considering all the shuntCells?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes you're right!

single-line-diagram-core/src/main/java/com/powsybl/sld/layout/ImplicitCellDetector.java
**/
private void detectAndTypeShunt(VoltageLevelGraph graph, Cell cell) {
private void detectAndTypeShunt(VoltageLevelGraph graph, ExternCell cell) {

List<Node> externalNodes = graph.getNodes()
.stream()
.filter(node -> !cell.getNodes().contains(node))
.collect(Collectors.toList());

Copy link
Contributor

Choose a reason for hiding this comment

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

(on the old code!) )wouldn't it be more efficient to do:
List externalNodes = new ArrayList<>(graph.getNodes());
externalNodes.removeAll(cell.getNodes());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, contains and remove are quite bad when it comes to (large) list, but let's see! if we call M the number of nodes in the cell and N the number of nodes in the graph:

  • contains checks all the elements in the list until the element is found so O(M), but here it is done N times, hence O(MN)
  • removeAll does a M loop of remove, each looking in the array of size N for the element until it is found, so O(MN) so far, hence equivalent to previous one, but removing the element copies the array starting from found index, so O(MN) again. At the end it's still O(MN), but it's probably worse because of copying M-times array of size N.

@flo-dup
Fix after reviewer's comment
e87d19f 
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

96.0% 96.0% Coverage
0.0% 0.0% Duplication

@flo-dup flo-dup merged commit b9f3f5f into main Mar 10, 2022
@flo-dup flo-dup deleted the consecutive_shunts branch March 10, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenoitJeanson BenoitJeanson BenoitJeanson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@flo-dup @BenoitJeanson