Skip to content

feat: iterate over shortest path#155

Open
jaapschoutenalliander wants to merge 17 commits intomainfrom
feature/iter_path
Open

feat: iterate over shortest path#155
jaapschoutenalliander wants to merge 17 commits intomainfrom
feature/iter_path

Conversation

@jaapschoutenalliander
Copy link
Member

@jaapschoutenalliander jaapschoutenalliander commented Feb 12, 2026

Changes proposed in this PR include:

This pull request introduces a new feature for inspecting branch records along the shortest path enabling users to iterate through the branch objects that make up the shortest path between two nodes, rather than just the node sequence.

    def iter_branches_in_shortest_path(
        self, from_node_id: int, to_node_id: int, typed: bool = False
    ) -> Iterator[BranchArray]:
        """Returns the ordered active branches that form the shortest path between two nodes.

        Args:
            from_node_id (int): External id of the path start node.
            to_node_id (int): External id of the path end node.
            typed (bool): If True, each yielded branch is converted to its typed array via
                ``get_typed_branches``.

        Yields:
            BranchArray: branch arrays for each active branch on the path.

        Raises:
            MissingBranchError: If the graph reports an edge on the shortest path but no active branch is found.
        """

For discussion

  • How to handle three-winding-transformers in the path

ToDo

Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: jaapschoutenalliander <[email protected]>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request introduces a new feature for inspecting branch records along the shortest path in a power grid network. The main addition is the iter_branches_in_shortest_path method on the Grid class, which enables users to iterate through the actual BranchArray objects that make up the shortest path between two nodes, rather than just getting the node sequence.

Changes:

  • Added iter_branches_in_shortest_path method to iterate through branches on the shortest path between two nodes
  • Implemented support for three-winding transformers in path iteration with special lookup logic
  • Added comprehensive test coverage for the new functionality including typed and untyped modes
  • Updated documentation notebook with usage example

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
src/power_grid_model_ds/_core/model/grids/_search.py Core implementation of path iteration logic including helper functions for three-winding transformer lookup and active branch filtering
src/power_grid_model_ds/_core/model/grids/base.py Public API method wrapper and import additions for the new functionality
tests/unit/model/grids/test_search.py Comprehensive test cases covering basic usage, three-winding transformers, same-node paths, and typed mode
docs/examples/model/graph_examples.ipynb Documentation example showing how to use the new feature, plus notebook metadata updates

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +88 to +89


Copy link

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

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

Consider adding test coverage for the MissingBranchError case that would be raised at line 142 of _search.py. This would happen when the graph reports a path but no active branch exists between consecutive nodes, which could occur if the graph state is inconsistent with the branch data. A test would help ensure this error is raised with a clear message in such edge cases.

Suggested change
def test_iter_branches_in_shortest_path_missing_branch_raises(self, basic_grid, monkeypatch):
# Simulate an inconsistent state where the graph reports a path but
# there is no active branch between consecutive nodes in that path.
def fake_get_shortest_path(*args, **kwargs):
# Return a path with a node pair that has no active branch
return [101, 999], 1
monkeypatch.object(basic_grid.graphs.active_graph, "get_shortest_path", fake_get_shortest_path)
with pytest.raises(Exception):
list(basic_grid.iter_branches_in_shortest_path(101, 999))

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Jaap Schouten <[email protected]>
@jaapschoutenalliander jaapschoutenalliander changed the title Feature: iter path feat: iter path Feb 15, 2026
Signed-off-by: jaapschoutenalliander <[email protected]>
@jaapschoutenalliander jaapschoutenalliander marked this pull request as ready for review February 15, 2026 15:52
)


def _lookup_three_winding_branch(grid: "Grid", node_a: int, node_b: int) -> ThreeWindingTransformerArray:
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer having both _private function below the public one.
(This is also recommended in the book Clean Clode)

Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: Vincent Koppen <[email protected]>
@vincentkoppen vincentkoppen changed the title feat: iter path feat: iterate over shortest path Feb 27, 2026
@sonarqubecloud
Copy link

)


def _get_branches(grid: "Grid", from_node: int, to_node: int) -> BranchArray:
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather have _private functions below the functions that use them. This is also suggested in the book "Clean Code"

Copy link
Member

@Thijss Thijss left a comment

Choose a reason for hiding this comment

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

1 comment, but LGTM

@Thijss Thijss added the do-not-merge This should not be merged label Mar 20, 2026
@vincentkoppen
Copy link
Member

On hold for now:

  • We have simplified the example code within the workshop
  • Discuss whether we want to rename this function and only let it work typed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge This should not be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants