Layout with initial node positions

[zamarrenolm]

Please check if the PR fulfills these requirements (please use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

• 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)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Allow to specify the points where some equipment (voltage levels) should be placed by the layout algorithm.
This could be applied in different use-cases:

• Layout in stages,
• Extend neighborhood keeping the current layout,
• Provide geographic locations for some nodes,
• Provide an initial very simple non-random layout (circle based on adjacencies, spanning tree of graph, ...).

What is the current behavior? (You can also link to an open issue here)
The layout algorithm decides the placement of all the nodes in the graph.

What is the new behavior (if this is a feature change)?
The LayoutParameters accept a Map that gives, for some equipment ids, a Point. Currently the equipment ids are always interpreted as voltage levels ids, but the layout parameter information simply maps string identifiers to positions.
The new layout parameter is not specific of the default force layout algorithm.

Does this PR introduce a breaking change or deprecate an API? If yes, check the following:
The NetworkAreaDiagram has been extended to allow direct calls to layout, without the need to produce an SVG output.

[flo-dup]

As said in powsybl/powsybl-dev-tools#59 I think we can get rid of this layout methods

[zamarrenolm]

They may be useful if we do not want to build the final visualization (SVG) or have an alternative visualization?

[flo-dup]

Yes indeed! But as said in powsybl/powsybl-dev-tools#59 (let's continue the discussion here, it's more appropriate!) shouldn't we rather add this in the Layout interface then? As the return value of Layout::run

[zamarrenolm]

Agree, you are right.

But I think then we will need to expose the graph built by the Diagram (or return a layout algorithm that has been already initialized with the right graph). So the user would see something like:

Diagram d = new Diagram(...);
Graph g = d.buildGraph();
Positions p = d.layout(graph).run();

Or

Diagram d = new Diagram(...);
Positions p = d.layout().run();

[flo-dup]

In fact you can do something like that already, similarly to what is done in NetworkAreaDiagram::draw method, as the graph builder is exposed. If you except the fact that Layout::run is not returning the positions (yet).

powsybl-network-area-diagram/src/main/java/com/powsybl/nad/NetworkAreaDiagram.java

Lines 136 to 138 in 996e2ce

	Graph graph = new NetworkGraphBuilder(network, voltageLevelFilter, idProvider).buildGraph();
	layoutFactory.create().run(graph, layoutParameters);
	new SvgWriter(svgParameters, styleProvider, labelProvider).writeSvg(graph, writer);

In fact this question meets the question of what is a Diagram object, and of whether it is of any use to have that kind of object. Before you shared this I was more thinking of changing NetworkAreaDiagram class to a utility class with only static methods (like the SingleLineDiagram one). Indeed we could think of the diagram as a graph with calculated positions, which are within the graph in fact. But if we separate the graph from the positions it would give more sense to a diagram object.

[zamarrenolm]

I think it is a good idea that the Diagram encapsulates how the graph has to be built. It has been already created or configured to know about the voltageLevelFilter and idProvider.
Following the same reasoning, I think it is also ok that Diagram is the owner of the layout method that will be used, but it should expose it so the user could call directly the new methods we want to add to its API.
My proposal is:

Diagram diagram = new Diagram(...)
Graph graph = diagram.buildGraph();
Layout layout = d.getLayout(layoutParameters);
layout.setInitialNodePositions(...);
layout.setNodesWithFixedPosition(...);
Map<String, Point> positions = layout.run(graph);

What do you think?

[zamarrenolm]

Layout parameters could be passed directly to the run method of layout, like it is done now:

Diagram diagram = new Diagram(...)
Graph graph = diagram.buildGraph();
Layout layout = d.getLayout();
layout.setInitialNodePositions(...);
layout.setNodesWithFixedPosition(...);
Map<String, Point> positions = layout.run(graph, layoutParameters);

[zamarrenolm]

I agree with your vision of the Diagram object as the sum of a graph plus its positions.

I always face the dilemma of whether build a complex object that is able to do one or two things or build a "slim" object with complex methods (current draw variants on Diagram). What do you think about moving the information currently received in these draw methods to secondary constructors of the Diagram object itself?

[flo-dup]

I think we forgot the topic of this PR on the way while discussing :) Let's keep this discussion for another PR / issue, where we'd decide whether we go towards a richer Diagram object or transform it to utility class with static functions. Could you thus remove the methods you added in NetworkAreaDiagram? Then we need to give the initial and fixed positions to the draw methods through the layoutFactory.

[flo-dup]

Just detailing why I'm not (yet) convinced by the changes you suggested. So far the diagram object does not really make sense, it is nothing more than a list of methods. In fact the constructors I wrote are a bit clumsy, as it's retaining things which would / should not be used twice. And yes, we could add parameters like the graph, the layout, the positions, so that it's really a meaningful object, but it's the opposite choice of what we did in powsybl-single-line-diagram.

In fact we used to have there something similar to what you suggest and we went back from this for several reasons:

• we didn't identify any usecase of calling twice draw or layout on a diagram object (other than development purposes), nor any usecase of playing with the diagram before drawing it
• just one line to call to draw a diagram, through a static method, instead of needing a tutorial to be able to use it
• not wondering which arguments should be in the constructor and which in the draw call, not wondering when handling a diagram whether its graph has already been build or not, whether the positions have already been computed or not

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
1 Security Hotspot
2 Code Smells

90.5% Coverage
0.0% Duplication

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

94.7% Coverage
0.0% Duplication