refactor(BaseGraph): improve performance of get_components #23
Conversation
…odel/power-grid-model-ds into feat/improve_get_components
vincentkoppen
force-pushed
the
feat/improve_get_components
branch
from
1de00b4 to
c4fb7bb
Compare
Signed-off-by: Vincent Koppen <[email protected]>
…odel/power-grid-model-ds into feat/improve_get_components
vincentkoppen
force-pushed
the
feat/improve_get_components
branch
from
c4fb7bb to
9f3bd5b
Compare
vincentkoppen
force-pushed
the
feat/improve_get_components
branch
from
ce92c86 to
9167327
Compare
Signed-off-by: Vincent Koppen <[email protected]>
vincentkoppen
force-pushed
the
feat/improve_get_components
branch
from
9167327 to
68adcd2
Compare
Signed-off-by: Vincent Koppen <[email protected]>
|
For some reason I do not see the performance boost when comparing the tests in https://github.com/PowerGridModel/power-grid-model-ds/blob/main/tests/performance/graph_performance_tests.py to main... |
| @@ -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]]: | |||
There was a problem hiding this comment.
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()
Signed-off-by: Thijs Baaijen <[email protected]>
Signed-off-by: Thijs Baaijen <[email protected]>
Thijss
force-pushed
the
feat/improve_get_components
branch
from
24b000d to
0351b12
Compare
|
| @@ -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() | |||
There was a problem hiding this comment.
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
Fixes issue: Performance of get_components
Changes proposed in this PR include:
Use the new
tmp_remove_nodeswithinget_components, this removes the need of a copy in the rustworkx implementation.Could you please pay extra attention to the points below when reviewing the PR: