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.

Sign up for GitHub

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

refactor(BaseGraph): improve performance of get_components #23

Open
wants to merge 8 commits into
base: feat/tmp-remove-nodes
Choose a base branch
from
+7 −10
Open

refactor(BaseGraph): improve performance of get_components #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a732914
Merge branch 'feat/tmp-remove-nodes' of https://github.com/PowerGridM…
vincentkoppen Jan 29, 2025
269182b
refactor(BaseGraph): improve performance of get_components
vincentkoppen Jan 29, 2025
9f3bd5b
Merge branch 'feat/tmp-remove-nodes' of https://github.com/PowerGridM…
vincentkoppen Jan 29, 2025
68adcd2
fix typing
vincentkoppen Jan 29, 2025
a40bedd
review Thijs
vincentkoppen Feb 1, 2025
f7e0085
fix graph imports for performance tests
Thijss Feb 3, 2025
0351b12
fix constants for graph performance tests
Thijss Feb 3, 2025
89d6341
Merge branch 'feat/tmp-remove-nodes' into feat/improve_get_components
Thijss Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/power_grid_model_ds/_core/model/graphs/models/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from contextlib import contextmanager
from typing import Generator

import numpy as np
from numpy._typing import NDArray

from power_grid_model_ds._core.model.arrays.pgm_arrays import Branch3Array, BranchArray, NodeArray
Expand Down Expand Up @@ -248,9 +247,10 @@ def get_all_paths(self, ext_start_node_id: int, ext_end_node_id: int) -> list[li

return [self._internals_to_externals(path) for path in internal_paths]

def get_components(self, substation_nodes: NDArray[np.int32]) -> list[list[int]]:
def get_components(self, substation_nodes: list[int]) -> list[list[int]]:
Copy link
Contributor

@Thijss Thijss Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

perhaps we can add the deprecation message for substation_nodes?

something like

if len(substation_nodes):
  logger.warning("use the context manager")  # message needs to better than this

warning should say intended use is as follows:

with self.tmp_remove_nodes(substation_nodes):
    components = graph.get_components()

"""Returns all separate components when the substation_nodes are removed of the graph as lists"""
internal_components = self._get_components(substation_nodes=self._externals_to_internals(substation_nodes))
with self.tmp_remove_nodes(substation_nodes):
internal_components = self._get_components()
return [self._internals_to_externals(component) for component in internal_components]

def get_connected(
Expand Down Expand Up @@ -345,7 +345,7 @@ def _get_shortest_path(self, source, target): ...
def _get_all_paths(self, source, target) -> list[list[int]]: ...

@abstractmethod
def _get_components(self, substation_nodes: list[int]) -> list[list[int]]: ...
def _get_components(self) -> list[list[int]]: ...

@abstractmethod
def _find_fundamental_cycles(self) -> list[list[int]]: ...
Expand Down
7 changes: 2 additions & 5 deletions src/power_grid_model_ds/_core/model/graphs/models/rustworkx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,8 @@ def _get_shortest_path(self, source: int, target: int) -> tuple[list[int], int]:
def _get_all_paths(self, source: int, target: int) -> list[list[int]]:
return list(rx.all_simple_paths(self._graph, source, target))

def _get_components(self, substation_nodes: list[int]) -> list[list[int]]:
no_os_graph = self._graph.copy()
Copy link
Contributor

@Thijss Thijss Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As @jaapschoutenalliander already pointed out: Surprisingly, there seems to be no performance increase. Verified it on my own laptop. Seems even slightly slower: 0.36 (this PR) vs. 0.33 (main) at 5000 nodes

I still think we can consider merging this because in my opinion it makes the implementation more readable and more consistent with the other methods.

Also, our performance test might not be representative enough (only 2 Substation nodes). I could also try a more representative performance comparison with my internal project at Alliander

for os_node in substation_nodes:
no_os_graph.remove_node(os_node)
components = rx.connected_components(no_os_graph)
def _get_components(self) -> list[list[int]]:
components = rx.connected_components(self._graph)
return [list(component) for component in components]

def _get_connected(self, node_id: int, nodes_to_ignore: list[int], inclusive: bool = False) -> list[int]:
Expand Down
2 changes: 1 addition & 1 deletion src/power_grid_model_ds/_core/model/grids/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def set_feeder_ids(grid: "Grid"):
601 | 101 | 204
"""
_reset_feeder_ids(grid)
feeder_node_ids = grid.node.filter(node_type=NodeType.SUBSTATION_NODE).id
feeder_node_ids = grid.node.filter(node_type=NodeType.SUBSTATION_NODE)["id"]
Thijss marked this conversation as resolved.
Show resolved Hide resolved
components = grid.graphs.active_graph.get_components(feeder_node_ids)
for component_node_ids in components:
component_branches = _get_active_component_branches(grid, component_node_ids)
Expand Down