Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions docs/source/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,5 @@ Return Iterator Types

retworkx.BFSSuccessors
retworkx.NodeIndices
retworkx.EdgeList
retworkx.WeightedEdgeList
86 changes: 45 additions & 41 deletions src/digraph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ use petgraph::visit::{
};

use super::dot_utils::build_dot;
use super::iterators::NodeIndices;
use super::iterators::{EdgeList, NodeIndices, WeightedEdgeList};
use super::{
is_directed_acyclic_graph, DAGHasCycle, DAGWouldCycle, NoEdgeBetweenNodes,
NoSuitableNeighbors, NodesRemoved,
Expand Down Expand Up @@ -661,11 +661,14 @@ impl PyDiGraph {
/// ``source`` and ``target`` are the node indices.
///
/// :returns: An edge list with weights
/// :rtype: list
pub fn edge_list(&self) -> Vec<(usize, usize)> {
self.edge_references()
.map(|edge| (edge.source().index(), edge.target().index()))
.collect()
/// :rtype: EdgeList
pub fn edge_list(&self) -> EdgeList {
EdgeList {
edges: self
.edge_references()
.map(|edge| (edge.source().index(), edge.target().index()))
.collect(),
}
}

/// Get edge list with weights
Expand All @@ -675,20 +678,20 @@ impl PyDiGraph {
/// payload of the edge.
///
/// :returns: An edge list with weights
/// :rtype: list
pub fn weighted_edge_list(
&self,
py: Python,
) -> Vec<(usize, usize, PyObject)> {
self.edge_references()
.map(|edge| {
(
edge.source().index(),
edge.target().index(),
edge.weight().clone_ref(py),
)
})
.collect()
/// :rtype: WeightedEdgeList
pub fn weighted_edge_list(&self, py: Python) -> WeightedEdgeList {
WeightedEdgeList {
edges: self
.edge_references()
.map(|edge| {
(
edge.source().index(),
edge.target().index(),
edge.weight().clone_ref(py),
)
})
.collect(),
}
}

/// Remove a node from the graph.
Expand Down Expand Up @@ -1160,21 +1163,22 @@ impl PyDiGraph {

let mut edges_to_add: Vec<(usize, usize, PyObject)> = Vec::new();
for dir in &DIRECTIONS {
let edges;
if dir == &petgraph::Direction::Outgoing {
edges = self.out_edges(u);
} else {
edges = self.in_edges(u);
}

for edge in edges {
for edge in self.graph.edges_directed(NodeIndex::new(u), *dir) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

let s = edge.source();
let d = edge.target();

if s == u {
edges_to_add.push((v, d, edge.weight().clone()));
if s.index() == u {
edges_to_add.push((
v,
d.index(),
edge.weight().clone_ref(py),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why clone_ref? Also, why does index() have to be added here but not before?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh, is it just because in the previous commits this hadn't been added yet, and so the type was just changed? I'd have thought some tests would fail in that case, though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was working before because it was using clone() instead of clone_ref() Before this PR in_edges() and out_edges() were returning a Vec(usize, usize, &PyObject) and edges_to_add was a Vec(usize, usize, &PyObject) (it's just being used as a temp storage Vec before its used later to add edges). So prior to the changing in_edges() and out_edges() this worked by the 2 usizes implement copy so can efficiently be used in place without moving, but to store &PyObject as a PyObject (which is neededt for creating a new edge in the graph) it needs to take ownership of the object being borrowed. We can't do *edge.weight() since it would move the weight out of the graph (and would fail) so we need to clone/copy it which for PyObject isn't a big deal because it just increments the ref counter.

The change I made here (besides switching to petgraph method edges_directed instead of using in_edges and out_edges) is to switch just switch from clone() to clone_ref(py). There isn't a functional difference between them as they both increment the python reference counter for the object, except that you give a gil handle to clone_ref if you have it: https://github.com/PyO3/pyo3/blob/v0.12.4/src/instance.rs#L196-L200 while clone() will queue it for when there is one it doesn't already have it: https://github.com/PyO3/pyo3/blob/v0.12.4/src/instance.rs#L517-L524. In the context of this method I don't think there is any performance difference since it has a gil reference, I just prefer it to be explicit and use clone_ref when we already have py

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh I missed the second half of the question. The index() calls are needed because the edges_to_add vec is defined as Vec<(usize, usize, PyObject)> and self.graph.edges_directed() returns an iterator yielding petgraph EdgeReference objects. Those objects return NodeIndex objects for source() and target() instead of usize ints. So to get the usize from the NodeIndex I had to call index() on it which returns a usize (since NodeIndex is just a wrapper around a usize anyay). This wasn't necessary before because in_edges() and out_edges returned a Vec<usize, usize, &PyObject)> and had already done the conversion from NodeIndex -> usize for us.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

got it!:)

));
} else {
edges_to_add.push((s, v, edge.weight().clone()));
edges_to_add.push((
s.index(),
v,
edge.weight().clone_ref(py),
));
}
}
}
Expand Down Expand Up @@ -1397,16 +1401,16 @@ impl PyDiGraph {
///
/// :returns: A list of tuples of the form:
/// ``(parent_index, node_index, edge_data)```
/// :rtype: list
/// :rtype: WeightedEdgeList
#[text_signature = "(self, node, /)"]
pub fn in_edges(&self, node: usize) -> Vec<(usize, usize, &PyObject)> {
pub fn in_edges(&self, py: Python, node: usize) -> WeightedEdgeList {
let index = NodeIndex::new(node);
let dir = petgraph::Direction::Incoming;
let raw_edges = self.graph.edges_directed(index, dir);
let out_list: Vec<(usize, usize, &PyObject)> = raw_edges
.map(|x| (x.source().index(), node, x.weight()))
let out_list: Vec<(usize, usize, PyObject)> = raw_edges
.map(|x| (x.source().index(), node, x.weight().clone_ref(py)))
.collect();
out_list
WeightedEdgeList { edges: out_list }
}

/// Get the index and edge data for all children of a node.
Expand All @@ -1418,16 +1422,16 @@ impl PyDiGraph {
///
/// :returns out_edges: A list of tuples of the form:
/// ```(node_index, child_index, edge_data)```
/// :rtype: list
/// :rtype: WeightedEdgeList
#[text_signature = "(self, node, /)"]
pub fn out_edges(&self, node: usize) -> Vec<(usize, usize, &PyObject)> {
pub fn out_edges(&self, py: Python, node: usize) -> WeightedEdgeList {
let index = NodeIndex::new(node);
let dir = petgraph::Direction::Outgoing;
let raw_edges = self.graph.edges_directed(index, dir);
let out_list: Vec<(usize, usize, &PyObject)> = raw_edges
.map(|x| (node, x.target().index(), x.weight()))
let out_list: Vec<(usize, usize, PyObject)> = raw_edges
.map(|x| (node, x.target().index(), x.weight().clone_ref(py)))
.collect();
out_list
WeightedEdgeList { edges: out_list }
}

/// Add new nodes to the graph.
Expand Down
43 changes: 23 additions & 20 deletions src/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use pyo3::types::{PyDict, PyList, PyLong, PyString, PyTuple};
use pyo3::Python;

use super::dot_utils::build_dot;
use super::iterators::NodeIndices;
use super::iterators::{EdgeList, NodeIndices, WeightedEdgeList};
use super::{NoEdgeBetweenNodes, NodesRemoved};

use petgraph::graph::{EdgeIndex, NodeIndex};
Expand Down Expand Up @@ -465,12 +465,15 @@ impl PyGraph {
/// ``source`` and ``target`` are the node indices.
///
/// :returns: An edge list with weights
/// :rtype: list
/// :rtype: EdgeList
#[text_signature = "(self)"]
pub fn edge_list(&self) -> Vec<(usize, usize)> {
self.edge_references()
.map(|edge| (edge.source().index(), edge.target().index()))
.collect()
pub fn edge_list(&self) -> EdgeList {
EdgeList {
edges: self
.edge_references()
.map(|edge| (edge.source().index(), edge.target().index()))
.collect(),
}
}

/// Get edge list with weights
Expand All @@ -480,21 +483,21 @@ impl PyGraph {
/// payload of the edge.
///
/// :returns: An edge list with weights
/// :rtype: list
/// :rtype: WeightedEdgeList
#[text_signature = "(self)"]
pub fn weighted_edge_list(
&self,
py: Python,
) -> Vec<(usize, usize, PyObject)> {
self.edge_references()
.map(|edge| {
(
edge.source().index(),
edge.target().index(),
edge.weight().clone_ref(py),
)
})
.collect()
pub fn weighted_edge_list(&self, py: Python) -> WeightedEdgeList {
WeightedEdgeList {
edges: self
.edge_references()
.map(|edge| {
(
edge.source().index(),
edge.target().index(),
edge.weight().clone_ref(py),
)
})
.collect(),
}
}

/// Remove a node from the graph.
Expand Down
157 changes: 157 additions & 0 deletions src/iterators.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,3 +183,160 @@ impl PySequenceProtocol for NodeIndices {
}
}
}

/// A custom class for the return of edge lists
///
/// This class is a container class for the results of functions that
/// return a list of edges. It implements the Python sequence
/// protocol. So you can treat the return as a read-only sequence/list
/// that is integer indexed. If you want to use it as an iterator you
/// can by wrapping it in an ``iter()`` that will yield the results in
/// order.
///
/// For example::
///
/// import retworkx
///
/// graph = retworkx.generators.directed_path_graph(5)
/// edges = graph.edge_list()
/// # Index based access
/// third_element = edges[2]
/// # Use as iterator
/// edges_iter = iter(edges)
/// first_element = next(edges_iter)
/// second_element = next(edges_iter)
///
#[pyclass(module = "retworkx")]
pub struct EdgeList {
pub edges: Vec<(usize, usize)>,
}

#[pyproto]
impl<'p> PyObjectProtocol<'p> for EdgeList {
fn __richcmp__(
&self,
other: &'p PySequence,
op: pyo3::basic::CompareOp,
) -> PyResult<bool> {
let compare = |other: &PySequence| -> PyResult<bool> {
if other.len()? as usize != self.edges.len() {
return Ok(false);
}
for i in 0..self.edges.len() {
let other_raw = other.get_item(i.try_into().unwrap())?;
let other_value: (usize, usize) = other_raw.extract()?;
if other_value != self.edges[i] {
return Ok(false);
}
}
Ok(true)
};
match op {
pyo3::basic::CompareOp::Eq => compare(other),
pyo3::basic::CompareOp::Ne => match compare(other) {
Ok(res) => Ok(!res),
Err(err) => Err(err),
},
_ => Err(PyNotImplementedError::new_err(
"Comparison not implemented",
)),
}
}
}

#[pyproto]
impl PySequenceProtocol for EdgeList {
fn __len__(&self) -> PyResult<usize> {
Ok(self.edges.len())
}

fn __getitem__(&'p self, idx: isize) -> PyResult<(usize, usize)> {
if idx >= self.edges.len().try_into().unwrap() {
Err(PyIndexError::new_err(format!("Invalid index, {}", idx)))
} else {
Ok(self.edges[idx as usize])
}
}
}

/// A custom class for the return of edge lists with weights
///
/// This class is a container class for the results of functions that
/// return a list of edges with weights. It implements the Python sequence
/// protocol. So you can treat the return as a read-only sequence/list
/// that is integer indexed. If you want to use it as an iterator you
/// can by wrapping it in an ``iter()`` that will yield the results in
/// order.
///
/// For example::
///
/// import retworkx
///
/// graph = retworkx.generators.directed_path_graph(5)
/// edges = graph.weighted_edge_list()
/// # Index based access
/// third_element = edges[2]
/// # Use as iterator
/// edges_iter = iter(edges)
/// first_element = next(edges_iter)
/// second_element = next(edges_iter)
///
#[pyclass(module = "retworkx")]
pub struct WeightedEdgeList {
pub edges: Vec<(usize, usize, PyObject)>,
}

#[pyproto]
impl<'p> PyObjectProtocol<'p> for WeightedEdgeList {
fn __richcmp__(
&self,
other: &'p PySequence,
op: pyo3::basic::CompareOp,
) -> PyResult<bool> {
let compare = |other: &PySequence| -> PyResult<bool> {
if other.len()? as usize != self.edges.len() {
return Ok(false);
}
let gil = Python::acquire_gil();
let py = gil.python();
for i in 0..self.edges.len() {
let other_raw = other.get_item(i.try_into().unwrap())?;
let other_value: (usize, usize, PyObject) =
other_raw.extract()?;
if other_value.0 != self.edges[i].0
|| other_value.1 != self.edges[i].1
|| self.edges[i].2.as_ref(py).compare(other_value.2)?
!= std::cmp::Ordering::Equal
{
return Ok(false);
}
}
Ok(true)
};
match op {
pyo3::basic::CompareOp::Eq => compare(other),
pyo3::basic::CompareOp::Ne => match compare(other) {
Ok(res) => Ok(!res),
Err(err) => Err(err),
},
_ => Err(PyNotImplementedError::new_err(
"Comparison not implemented",
)),
}
}
}

#[pyproto]
impl PySequenceProtocol for WeightedEdgeList {
fn __len__(&self) -> PyResult<usize> {
Ok(self.edges.len())
}

fn __getitem__(&'p self, idx: isize) -> PyResult<(usize, usize, PyObject)> {
if idx >= self.edges.len().try_into().unwrap() {
Err(PyIndexError::new_err(format!("Invalid index, {}", idx)))
} else {
Ok(self.edges[idx as usize].clone())
}
}
}
Loading