feat: improve downstream nodes performance with local search #19
Conversation
Signed-off-by: jaapschoutenalliander <[email protected]>
Thijss
left a comment
Thijss
left a comment
There was a problem hiding this comment.
Nice work and I think "sorted by distance" seems preserved. Perhaps we can add/alter a test to verify it? Current tests only seem to check results using sets
| def get_downstream_nodes(self, node_id: int, stop_node_ids: list[int], inclusive: bool = False) -> list[int]: | ||
| """Find all nodes connected to the node_id | ||
| args: | ||
| node_id: node id to start the search from | ||
| stop_node_ids: list of node ids to stop the search at | ||
| inclusive: whether to include the given node id in the result | ||
| returns: | ||
| list of node ids sorted by distance, downstream of to the node id | ||
| """ | ||
| downstream_nodes = self._get_downstream_nodes( | ||
| node_id=self.external_to_internal(node_id), | ||
| stop_node_ids=self._externals_to_internals(stop_node_ids), | ||
| inclusive=inclusive, | ||
| ) | ||
|
|
||
| return self._internals_to_externals(downstream_nodes) |
There was a problem hiding this comment.
In what way are the stop_node_ids still needed with the tmp_remove_nodes context implemented by @Thijss?
Isn't this equal to:
with graph.tmp_remove_node(stop_node_ids):
graph.get_downstream_nodes(node_id = node_id)
There was a problem hiding this comment.
Functionally it should be equal, but since we aren't modifying the graph here it could give significant better performance?
Perhaps it's a good idea to base this PR on #17 and add a performance comparison.
If the performance increases significantly we can merge the PR. Otherwise we can drop it.
There was a problem hiding this comment.
We could try that, the main performance improve should be in not using get_nearest_substation_node to get_connected on the whole grid. Technically using tmp_remove_nodes can achieve a similar result
There was a problem hiding this comment.
Issue when we want to do this is that we do not have the information about which substation is the feeder, so I do not think the tmp_remove_nodes will work (without additional complexity)
Signed-off-by: jaapschoutenalliander <[email protected]>
Signed-off-by: jaapschoutenalliander <[email protected]>
Co-authored-by: jaapschoutenalliander <[email protected]> Signed-off-by: Thijs Baaijen <[email protected]>
Co-authored-by: jaapschoutenalliander <[email protected]> Signed-off-by: Thijs Baaijen <[email protected]>
|
|
@jaapschoutenalliander shall we drop this PR in favor of #21 ? |
|
Agreed, since this solution won't cover all cases |
Performance:
Improve downstream nodes performanceChanges proposed in this PR include: