Clarify graph API regarding vertex indices and its use in node breaker topology

[sylvlecl]

• Do you want to request a feature or report a bug?

API improvement.

• What is the current behavior?

See PR #775: the node breaker topology uses the UndirectGraph interface to represent the topology. When creating it, it starts by setting a "node count" before actually adding those nodes. A problem arises when nodes indices are not contiguous, for example after removing some equipments. In that case, the index of some vertices can overflow the allocated array. The PR fixes this by ensuring that the array is extended in that case, but the node breaker API remains ambiguous.

• What is the expected behavior?

The API must be clarified:

• Are vertices indices required to be contiguous ?
• Should the setNodeCount method be deleted, considered as handled by the graph implementation? Otherwise, what should it do ? If only allocation with a "guess", rename it ? If we remove it, should we add a method to ensure memory efficiency once the voltage level is "stabilized" ? (compact)

• What is the motivation / use case for changing the behavior?

Clarify the API to avoid bugs when using it, for example by underallocating vertices, or by looping on vertices indices from 0 to N whereas they are not contiguous.

• Please tell us about your environment: PowSyBl Version: 2.5.0-SNAPSHOT

• Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, eg. stackoverflow, spectrum, etc)

Follows bug fix in PR #775 .

[mathbagu]

The graph API has not setNodeCount method. This method is in NodeBreakerView interface.

From my point of view the behavior of the graph is fine, but I agree to add javadoc to clarify things:

• the vertices index are not contiguous. As we are able to remove a vertex, it creates a discontinuity. Adding a compact method seems very hard to implement because the NodeTerminal contains a vertex index and we have to maintain the consistency. If we add such method, we should do it in NodeBreakerView. I’m not sure that it is needed

About for-loop, we have to clarify that:

• getVertexCount returns the number of vertices in the graph
• getMaxVertex returns the biggest index of a vertex
• to iterate over the vertices, use should use getVertices().

I think we should add a way to create a vertex with a specific index. This is what I propose in the PR #775. But we should not change the behavior of addEdge(): this method should throw an exception if one of its ends doesn’t exist.

From my point of view, we can change the meaning of the setNodeCount method. If we care about memory consumption and user friendly API, we should remove this method and create vertices when they are needed. If we care about memory consumption, the setNodeCount method should not allocate vertex but reserve memory (creating unused vertices). If we want to change nothing, we should remove the call to removeIsolatedVertex when we remove a connectable.

[sylvlecl]

Indeed, I fixed the confusion between the graph API and its use in node breaker topology creation, in the issue text.

For the "compact" method, I did not mean to re-index the vertices, but only resize the array to not waste space with overallocation. But thinking again it does not seem very useful, in any case today the real capacity of the underlying array is managed by ArrayList.

--> I vote for removal of the setNodeCount to avoid confusions.