-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move collect_bicolor_runs()
to rustworkx-core
#1186
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-core
and modifies the python-facingcollect_bicolor_runs
to 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.