Skip to content

fix: bugfix three winding transformers in graphs#198

Open
vincentkoppen wants to merge 17 commits into
pgm/add_tmp_remove_branchesfrom
fix/three-winding-with-cycle-representation
Open

fix: bugfix three winding transformers in graphs#198
vincentkoppen wants to merge 17 commits into
pgm/add_tmp_remove_branchesfrom
fix/three-winding-with-cycle-representation

Conversation

@vincentkoppen
Copy link
Copy Markdown
Member

@vincentkoppen vincentkoppen commented Mar 20, 2026

Depends on #267 to be merged first
Fixes #167

There were multiple ways to solve this problem:

  • Keep 2 representations of the graph internally and use the "correct" one for each algorithm
    Disadvantage on memory
  • Represent a three-winding transformer as a star and adjust algorithms where necessary.
    Disadvantage: a lot of current algorithms/queries become harder, since you always need to hop / remove the node.
  • Keep representation as is (a cycle), but fix the algorithms that are hit by the problem (currently only 2 that share a lot of similarity)

Neither option is perfect, but for now keeping it mostly as is and fixing the two algorithms that were incorrect seems like the easiest/cleanest option.
For these algorithms we:

  • Remove one edge of the cycle (if all three active)
  • Perform normal algorithm
  • If any of the returned paths contains all three nodes of the three winding transformer after each other, we know that in reality we would have taken the removed edge instead of "walking through the three winding trafo". So we replace those
  • Return to user
    This way we do not return additional paths that are not there, nor do we see cycles that are not there (since we removed the cycles introduced by the three winding trafo's).

Later if we add even more algorithm we can always revisit again en see whether one of the other option becomes a better solution.

…into fix/three-winding-with-cycle-representation
Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
DCO Remediation Commit for Vincent Koppen <vincent.koppen@alliander.com>

I, Vincent Koppen <vincent.koppen@alliander.com>, hereby add my Signed-off-by to this commit: b305453
I, Vincent Koppen <vincent.koppen@alliander.com>, hereby add my Signed-off-by to this commit: 9701f0c
I, Vincent Koppen <vincent.koppen@alliander.com>, hereby add my Signed-off-by to this commit: affb900
I, Vincent Koppen <vincent.koppen@alliander.com>, hereby add my Signed-off-by to this commit: ed61f71

Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
@vincentkoppen vincentkoppen changed the title wip: bugfix three winding transformers in graphs fix: bugfix three winding transformers in graphs Apr 10, 2026
@vincentkoppen vincentkoppen marked this pull request as ready for review April 10, 2026 15:08
vincentkoppen and others added 4 commits April 10, 2026 17:11
Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 8, 2026

Copy link
Copy Markdown
Member

@jaapschoutenalliander jaapschoutenalliander left a comment

Choose a reason for hiding this comment

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

Happy with the suggested solution for now. Some input on the implementation but mainly looks good

source=self.external_to_internal(ext_start_node_id),
target=self.external_to_internal(ext_end_node_id),
)
branches_to_remove = self._branches_to_remove_from_three_winding_transformers()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest putting the boiler plate to remove the three winding transformers and re-add them in a re-usable wrapper. If there are more algorithms that need it they can easily do that:

something like

    def _run_with_three_winding_correction(
        self,
        algorithm: Callable[[], list[list[int]]],
    ) -> list[list[int]]:
        branches_to_remove = self._branches_to_remove_from_three_winding_transformers()
        with self.tmp_remove_branches(branches_to_remove):
            result = algorithm()
        if branches_to_remove:
            return self._squash_paths_inside_three_winding_transformers(result)
        return result

Copy link
Copy Markdown
Member

@Thijss Thijss May 11, 2026

Choose a reason for hiding this comment

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

Seems like a good idea. I guess for readability we could even make a contextmanager.

with self._three_winding_correction():
    ...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will experiment with this, for the current functionality this works. But only since the two algorithms that we need to "fix" happen to have the same output (list[list[int]]). If we have a third algorithm later on that needs fixing with a different output type, we might have to revisit.
--> Think I can also solve this by also applying a "post-processs" function that we can now set as default to the current one

Copy link
Copy Markdown
Member Author

@vincentkoppen vincentkoppen Jun 1, 2026

Choose a reason for hiding this comment

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

I don't think a context manager can work, unless we do not include the _squash_paths_inside_three_winding_transformers part in it.
We do not get access to the result of the function that is called within the context manager, which is needed for that function.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@Thijss and @jaapschoutenalliander I have simplified the setup I think, let me know what you think! :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice, happy with this approach

Comment thread src/power_grid_model_ds/_core/model/graphs/models/base.py Outdated
Comment thread src/power_grid_model_ds/_core/model/graphs/models/base.py Outdated
Comment thread src/power_grid_model_ds/_core/model/graphs/models/base.py Outdated
Comment thread src/power_grid_model_ds/_core/model/graphs/models/base.py
vincentkoppen and others added 3 commits June 1, 2026 10:15
Co-authored-by: Jaap Schouten <58551444+jaapschoutenalliander@users.noreply.github.com>
Signed-off-by: Vincent Koppen <53343926+vincentkoppen@users.noreply.github.com>
Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
…h-cycle-representation

Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
@vincentkoppen vincentkoppen changed the base branch from main to pgm/add_tmp_remove_branches June 1, 2026 08:25
Signed-off-by: Vincent Koppen <vincent.koppen@alliander.com>
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 1, 2026

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.

[BUG] Three winding transformers are represented as cycles in the graph

3 participants