Move collect_bicolor_runs() to rustworkx-core
#1186
Conversation
| graph: G, | ||
| filter_fn: F, | ||
| color_fn: C, | ||
| ) -> Result<Vec<Vec<&<G as Data>::NodeWeight>>, CollectBicolorSimpleError<E>> //OG type: PyResult<Vec<Vec<PyObject>>> |
There was a problem hiding this comment.
Instead of returning NodeWeight here I think it would be simpler to just return something like:
impl Iterator<Item=Vec<G::NodeId>>
(or something like that I might have made a mistake in the exact syntax) Or alternatively a Vec<Vec<G::NodeId>>
The idea is to return an iterator of node indices for each run. This isn't exactly what the python api was returning, but retunring the node ids will let us easily iterate and map that to the weights for python, but for other rust consumer (especially Qiskit in the future) this will be lighter weight to work with because it's a simple vec get to go from an index to a weight.
Then using an iterator would be a more rust native interface to return an iterator of the runs
rustworkx-core/src/lib.rs
Outdated
| @@ -73,6 +73,7 @@ pub type Result<T, E = Infallible> = core::result::Result<T, E>; | |||
| pub mod bipartite_coloring; | |||
| /// Module for centrality algorithms. | |||
| pub mod centrality; | |||
| pub mod collect_bicolor_runs; | |||
There was a problem hiding this comment.
This was moved from under pub mod connectivitiy when I ran cargo fmt, I guess that they must be in alphabetical order (I will move it back to coloring algorithms).
Pull Request Test Coverage Report for Build 9289754515Details
💛 - Coveralls |
ElePT
changed the title
collect_bicolor_runs() to rustworkx-corecollect_bicolor_runs() to rustworkx-core
| /// Define custom error classes for collect_bicolor_runs | ||
| #[derive(Debug)] | ||
| pub enum CollectBicolorError<E: Error> { | ||
| DAGHasCycle, | ||
| CallableError(E), | ||
| } | ||
|
|
||
| impl<E: Error> Error for CollectBicolorError<E> {} | ||
|
|
||
| impl<E: Error> Display for CollectBicolorError<E> { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| CollectBicolorError::DAGHasCycle => fmt_dag_has_cycle(f), | ||
| CollectBicolorError::CallableError(ref e) => fmt_callable_error(f, e), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn fmt_dag_has_cycle(f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| write!(f, "Sort encountered a cycle") | ||
| } | ||
|
|
||
| fn fmt_callable_error<E: Error>(f: &mut Formatter<'_>, inner: &E) -> std::fmt::Result { | ||
| write!(f, "The function failed with: {:?}", inner) | ||
| } |
There was a problem hiding this comment.
I am not 100% sure that defining these custom error classes is the best way to go, maybe it's too cumbersome? It took me a while to figure out how to convert to and from CollectBicolorError::CallableError andPyErr, and I ended up using this function. The custom errors have also made it difficult to come up with a simple example to add to the docs, because I always need to define the type hint for the error, but this might be also lack of experience on my side.
There was a problem hiding this comment.
So I think this pattern is fine, especially if there are multiple sources of potential error. Although in the case there really is two conditions to think about, either the input graph is not a DAG (i.e. has a cycle) or the callback has an error. To simplify the function what I've typically been doing is using None to represent a graph with a cycler and then using a fully generic E for the error type from the callback and the functions result. That lets you use ? to pass through the callback's error directly to the caller and still handle the error case without too much extra complexity. This would basically be changing the function signature to be like:
pub fn collect_bicolor_runs<G, F, C, B, E>(
graph: G,
filter_fn: F,
color_fn: C,
) -> Result<Option<Vec<Vec<G::NodeId>>>, E>
where
F: Fn(&<G as Data>::NodeWeight) -> Result<Option<bool>, E>,
C: Fn(&<G as Data>::EdgeWeight) -> Result<Option<usize>,E>,
G: NodeIndexable
+ IntoNodeIdentifiers
+ IntoNeighborsDirected
+ IntoEdgesDirected
+ Visitable
+ DataMap,
<G as GraphBase>::NodeId: Eq + Hash,
{The only wrinkle in this case is because you have 2 callbacks a user needs to ensure filter_fn and color_fn return the same error type. But that should be ok in practice (and for rustworkx's python crate it will just be a PyErr).
| @@ -599,3 +766,218 @@ mod test_lexicographical_topological_sort { | |||
| assert_eq!(result, Ok(Some(vec![nodes[7]]))); | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
| mod test_collect_bicolor_runs { | |||
There was a problem hiding this comment.
I tried my best migrating the python tests to rust but I skipped test_color_with_ignored_edge and test_two_colors_with_barrier.
This is a first attempt at solving #1166, the PR introduces a new
collect_bicolor_runs()function inrustworx-coreand modifies the python-facingcollect_bicolor_runsto use the core function under the hood (no user-facing changes).The current function returns a
<Vec<Vec<G::NodeId>>, I could not find a way to implement the iterator mentioned in the original issue because I kept getting "the size cannot be known at compilation time", but I am open to implementation suggestions.