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

corrects Move connectable implementation in order to execute tcks on the network store server #447

Merged
merged 6 commits into from
Sep 17, 2024

Conversation

Mathieu-Deharbe
Copy link
Contributor

@Mathieu-Deharbe Mathieu-Deharbe commented Sep 9, 2024

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)

What kind of change does this PR introduce?
Corrects a bug in moveConnectable : the moved connectable wasn't correctly updated.

It will fix the tck tests executed here powsybl/powsybl-network-store-server#61

What is the current behavior?
The tests here powsybl/powsybl-network-store-server#61 have been overriden by an emtpy version that should be removed.

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

  • Yes
  • No

Signed-off-by: Mathieu DEHARBE <[email protected]>
Signed-off-by: Mathieu DEHARBE <[email protected]>
Signed-off-by: Mathieu DEHARBE <[email protected]>
InjectionAttributes attr = attributesGetter.apply(res);
attr.setConnectableBus(busId);
attr.setBus(connected ? busId : null);
attr.setNode(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you explain the reason to set null value for node in this case ?

Copy link
Contributor Author

@Mathieu-Deharbe Mathieu-Deharbe Sep 11, 2024

Choose a reason for hiding this comment

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

Because an equipment may be connected by Node (in node breaker view) of by bus (in bus breaker view). When you affect one of those values the other makes no sense anymore (and this may cause problems of you reaffect to the previous value, like in the unit tests).

However this brings a good question : may an equipment be connected to a bus and a node simultaneously in the same voltage level ? In the unit test the bus breaker view and the node breaker view were in different voltage levels so the previous value had to be nulled.

I don't really know the answer, what do you say @geofjamg ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Geoffroy told me that a voltage level can only be in node breaker OR bus breaker. Therefore it is impossible to be connected at the same to a node and to a bus in the same voltage level. And therefore setting the other to null should be fine.

getAbstractIdentifiable().updateResource(res -> {
InjectionAttributes attr = attributesGetter.apply(res);
attr.setConnectableBus(null);
attr.setBus(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

the same remarks for Bus and connectableBus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and same answer as well.

Copy link

@etiennehomer etiennehomer merged commit b240dab into main Sep 17, 2024
4 checks passed
@etiennehomer etiennehomer deleted the move-connectable-tck branch September 17, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants