Add in and out edge indices functions #1369
Conversation
ThisuraGallage
changed the title
Pull Request Test Coverage Report for Build 12892224234Details
💛 - Coveralls |
There was a problem hiding this comment.
The current implementation looks good to me. I left some minor nitpick comments but they are not blocking.
What is going to be more work is adding even more methods...
PyGraphshould have the same methods even if the implementation is just callingincident_edgesfor consistencyPyDiGraphandPyGraphcould use aout_edge_indices
So that is 3 more methods which were in the original request but are useful to maintain some coherence accross the API
| @@ -1423,6 +1423,7 @@ class PyDiGraph(Generic[_S, _T]): | |||
| def in_edges(self, node: int, /) -> WeightedEdgeList[_T]: ... | |||
| def incident_edge_index_map(self, node: int, /, all_edges: bool = ...) -> EdgeIndexMap: ... | |||
| def incident_edges(self, node: int, /, all_edges: bool = ...) -> EdgeIndices: ... | |||
| def in_edge_indices(self, node: int, /) -> EdgeIndices: ... | |||
There was a problem hiding this comment.
Please add this method to PyGraph as well. It is dumb BUT it doesn't hurt to keep the API consistent.
It will just return all edges for the PyGraph case and you should note that in the docstring
src/digraph.rs
Outdated
| EdgeIndices { | ||
| edges: self | ||
| .graph | ||
| .edges_directed(node_index, petgraph::Direction::Incoming) |
There was a problem hiding this comment.
| .edges_directed(node_index, petgraph::Direction::Incoming) | |
| .edges_directed(node_index, dir) |
| /// :rtype: EdgeIndices | ||
| #[pyo3(text_signature = "(self, node, /)")] | ||
| pub fn in_edge_indices(&self, node: usize) -> EdgeIndices { | ||
| let node_index = NodeIndex::new(node); |
There was a problem hiding this comment.
| let node_index = NodeIndex::new(node); | |
| let node_index = NodeIndex::new(node); | |
| let dir = petgraph::Direction::Incoming; |
This is a nit the other functions just use this style. I didn't write them but I am ok sticking with it
| /// :returns: A list of the incoming edge indices to a node in the graph | ||
| /// :rtype: EdgeIndices | ||
| #[pyo3(text_signature = "(self, node, /)")] | ||
| pub fn in_edge_indices(&self, node: usize) -> EdgeIndices { |
There was a problem hiding this comment.
Another suggestion is adding out_edge_indices which is the same function but with dir = petgraph::Direction::Outgoing
| @@ -813,6 +813,23 @@ def test_incident_edges_all_edges(self): | |||
| res = graph.incident_edges(node_d, all_edges=True) | |||
| self.assertEqual([2, 1], res) | |||
|
|
|||
| def test_in_edge_indices(self): | |||
There was a problem hiding this comment.
I know you copied this test case from above but in hindsight we can improve it
tests/digraph/test_edges.py
Outdated
| graph.add_edge(node_a, node_c, "edge a") | ||
| graph.add_edge(node_b, node_d, "edge b") | ||
| graph.add_edge(node_d, node_c, "edge c") | ||
| res = graph.in_edge_indices(node_c) | ||
| self.assertEqual([2, 0], res) |
There was a problem hiding this comment.
| graph.add_edge(node_a, node_c, "edge a") | |
| graph.add_edge(node_b, node_d, "edge b") | |
| graph.add_edge(node_d, node_c, "edge c") | |
| res = graph.in_edge_indices(node_c) | |
| self.assertEqual([2, 0], res) | |
| edge_ac = graph.add_edge(node_a, node_c, "edge a") | |
| graph.add_edge(node_b, node_d, "edge b") | |
| edge_dc = graph.add_edge(node_d, node_c, "edge c") | |
| res = graph.in_edge_indices(node_c) | |
| self.assertEqual([edge_dc, edge_ac], res) |
ThisuraGallage
changed the title
This PR introduces a new method, in_edge_indices, to the rustworkx.PyDiGraph class. The method returns the indices of incoming edges for a specified node.
#1360
I'll add the documentation once the function signature is finalized.