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.

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

Add nodeBreakerView.hasAttachedEquipment(node) method + Ensure an IIDM node number is valid before trying to use it in CGMES conversion #1233

Merged
merged 5 commits into from
Apr 2, 2020

Conversation

zamarrenolm
Copy link
Member

Signed-off-by: Luma Zamarreño zamarrenolm@aia.es

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, ...)
Bug fix for conversion of isolated connectivity nodes in CGMES.

What is the current behavior? (You can also link to an open issue here)
CGMES data contains an isolated connectivity node (no terminals attached to it, no related equipment).
Space for the connectivity node is reserved at IIDM node-breaker graph (a vertex is reserved).
During the conversion, the reserved vertex is never used (no equipment is attached to it).
After all equipment has been converted, voltages and angles from SV are put on the resulting IIDM data.
The CGMES SV data contains voltage and angle values for the connectivity node.
But the corresponding node in IIDM node-breaker graph is not valid.
An exception is raised.

What is the new behavior (if this is a feature change)?
Before trying to use the node from IIDM node-breaker graph, it is verified that the node number is valid.
To do it, we check that the nodes returned by the node-breaker view in IIDM contain the node number reserved.
There is no severe performance penalty on adding the check. It has been tested on a network with more than 14000 values for voltages. Without the check, all voltages and angles were set in 1400 ms. After adding the check, the cost is 1600 ms.

Copy link
Contributor

@miovd miovd left a comment

Choose a reason for hiding this comment

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

Not sure if the correction should be here... I don't think topo.getTerminal(node) should throw an exception if the node exists but is not attached to an equipment.

private boolean iidmNodeIsValid(VoltageLevel.NodeBreakerView topo, int iidmNode) {
// A node is valid only if it has some equipment attached
// The list of valid nodes is obtained from the node-breaker view
// We assume the returned list is sorted
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we should assume that, or we should update the documentation.
Anyway, is it the best way to check if a terminal is associated to this node: maybe we should add a convenient method to get an Optional<Terminal> from a node?

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 not enough because a node is still valid if an internal connection or a switch is attached to it (with no terminal)... I added a convenient method to check if there is any equipment attached to the node at NodeBreakerView level to delete this method.

@miovd
Copy link
Contributor

miovd commented Mar 24, 2020

You can now use the method getOptionalTerminal(node) merged since PR #1235

@miovd
Copy link
Contributor

miovd commented Mar 27, 2020

@zamarrenolm I changed your PR following fixes in VoltageLevel API, do you agree with this correction?

.setName(vlName)
.setNominalV(nominalVoltage)
.setTopologyKind(context.nodeBreaker() ? TopologyKind.NODE_BREAKER : TopologyKind.BUS_BREAKER)
.add();
Copy link
Contributor

Choose a reason for hiding this comment

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

@zamarrenolm you should configure your IDE to set to 8 spaces the continuous indentation.

zamarrenolm and others added 4 commits March 31, 2020 14:43
Signed-off-by: Luma Zamarreño <zamarrenolm@aia.es>
…entations

Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>
Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>
Signed-off-by: RALAMBOTIANA MIORA <miora.ralambotiana@rte-france.com>
@miovd miovd force-pushed the cgmes_conversion_ensure_valid_iidm_node branch from 07155b5 to 9ea56a5 Compare March 31, 2020 12:44
@miovd miovd added the bug label Mar 31, 2020
@zamarrenolm
Copy link
Member Author

@zamarrenolm I changed your PR following fixes in VoltageLevel API, do you agree with this correction?

Totally agree. I have checked it with the test case that was giving me problems.

@miovd miovd changed the title Ensure an IIDM node number is valid before trying to use it in CGMES conversion Add hasAttachedEquipment(node) method + Ensure an IIDM node number is valid before trying to use it in CGMES conversion Apr 2, 2020
@miovd miovd changed the title Add hasAttachedEquipment(node) method + Ensure an IIDM node number is valid before trying to use it in CGMES conversion Add nodeBreakerView.hasAttachedEquipment(node) method + Ensure an IIDM node number is valid before trying to use it in CGMES conversion Apr 2, 2020
@sonarcloud
Copy link

sonarcloud bot commented Apr 2, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

38.5% 38.5% Coverage
0.0% 0.0% Duplication

@miovd miovd merged commit e1df3ba into master Apr 2, 2020
@miovd miovd deleted the cgmes_conversion_ensure_valid_iidm_node branch April 2, 2020 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants