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

Fix busbar section minimal tripping #1142

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

flo-dup
Copy link
Contributor

@flo-dup flo-dup commented Dec 4, 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)

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?
The tripping corresponding to a bbs contingency with no propagation adds the traversed terminals in the neigbours list.

What is the new behavior (if this is a feature change)?
The tripping corresponding to a bbs contingency with no propagation adds the visited terminals in the traversed terminals set.

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

  • Yes
  • No

Signed-off-by: Florian Dupuy <[email protected]>
@flo-dup
Copy link
Contributor Author

flo-dup commented Dec 4, 2024

I still need to add a unit test!

@flo-dup
Copy link
Contributor Author

flo-dup commented Jan 15, 2025

To me there's no easy way to include a unit test, could we merge it like this? The unit test would be really complex to write for just having swapped two arguments, I think this is not worth the effort.

The original mistake of adding the traversed terminal to the list of neighbouring terminals (to later visit) instead of the set of traversed terminals (to disconnect) has no impact at the end. Indeed, the neighbouring terminals are later visited and added to the set of traversed terminals.

@vidaldid-rte
Copy link
Collaborator

To me there's no easy way to include a unit test, could we merge it like this? The unit test would be really complex to write for just having swapped two arguments, I think this is not worth the effort.

The original mistake of adding the traversed terminal to the list of neighbouring terminals (to later visit) instead of the set of traversed terminals (to disconnect) has no impact at the end. Indeed, the neighbouring terminals are later visited and added to the set of traversed terminals.

I realized that there is no simple test in ContingencyTrippingTest that goes through the line that uses traversedTerminals (nbv.getOptionalTerminal(nodeAfter).ifPresent(traversedTerminals::add);)
I'll try to catch you to understand how to write one and also understand what should be the expected result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants