Resolve issue #1029 #1170
Resolve issue #1029 #1170
Conversation
|
|
There was a problem hiding this comment.
I will try to review this when I have time. I am sorry we mislabeled #1029, maybe we underestimated the effort to implement the algorithm?
Nevertheless, congrats on completing the work and submitting the PR! It is a big accomplishment
|
For convenience, I think I need to explain my idea why I perform the subgraph creation and the lifted graph construction as in the |
| IntoNodeIdentifiers, IntoNodeReferences, NodeIndexable, Visitable, | ||
| }; | ||
| use petgraph::Undirected; | ||
| use std::fmt::Debug; |
There was a problem hiding this comment.
You should remove std::fmt::Debug trait from here and the function signatures
| let source_name = format!("{}", graph.to_index(edge.source())); | ||
| let target_name = format!("{}", graph.to_index(edge.target())); |
There was a problem hiding this comment.
Same comment as above about hashing
There was a problem hiding this comment.
I'm wondering without the name, how can we know which node is which after putting these into calculations related to the subgraphs and the lifted graph. Imagine NodeIndex(1) in the original graph (user input), which might be NodeIndex(0) in a subgraph, and NodeIndex(2) in the lifted graph. I can only think of mapping these NodeIndex(i) from one graph with respect to another, and apparently we still need have to use HashMap. Does it sound good?
There was a problem hiding this comment.
You can create a custom struct to store data e.g. a pair of integers. And then implement the hash trait with derive https://doc.rust-lang.org/std/hash/trait.Hash.html#implementing-hash.
Strings contain at least 24 bits even if they are empty. So we should find better representations to store intermediate data in a more compact way. You don’t want string hashing to be the bottleneck of the algorithm
| .map(|component| { | ||
| let mut subgraph = Graph::<String, i32>::new(); | ||
| // Create map index to NodeIndex of the nodes in each component | ||
| let mut name_idx_map: HashMap<String, usize> = HashMap::new(); |
There was a problem hiding this comment.
This is not necessary, you should be able to use the NodeId directly because it is hashable. Or use the usize as a hash
| for &node_id in &component { | ||
| let node_index = graph.to_index(node_id); | ||
| // Create the name of the node in subgraph from the original graph | ||
| let node_name = format!("{}", node_index).trim_matches('"').to_string(); |
There was a problem hiding this comment.
Same comment as above about hashing
| let mst = min_spanning_tree(&graph); | ||
| let mut mst_edges: Vec<(usize, usize)> = Vec::new(); | ||
| for element in mst { | ||
| // println!("Element: {:?}", element); |
There was a problem hiding this comment.
Remove the debug statements
| graph: G, | ||
| orth: HashSet<(usize, usize)>, | ||
| mut weight_fn: F, | ||
| name_idx_map: &HashMap<String, usize>, |
There was a problem hiding this comment.
We'll need to propagate the hashing changes to _min_cycle as well
| fn _min_cycle_basis<G, F, E>( | ||
| graph: G, | ||
| weight_fn: F, | ||
| name_idx_map: &HashMap<String, usize>, |
There was a problem hiding this comment.
We'll need to propagate the hashing changes to _min_cycle as well
| for node in path { | ||
| let node_name = gi.node_weight(node).unwrap(); | ||
| if node_name.contains("_lifted") { | ||
| let original_node_name = node_name.replace("_lifted", ""); |
There was a problem hiding this comment.
If you need to add additional information to the hash you can also hash tuples in Rust. Or you can come up with a custom struct and write a hasher for it
| let nodeidx = NodeIndex::new(node); | ||
| let lifted_node = gi_name_to_node_index[&lifted_node_name]; | ||
| let lifted_nodeidx = NodeIndex::new(lifted_node); | ||
| let result = astar( |
There was a problem hiding this comment.
Should probably just be a call for dijkstra given the A* heuristic is a constant
gluonhiggs
force-pushed
the
resolve_issue_#1029
branch
from
dfbc364
to
f7716ac
Compare
|
I have added some modifications based on your advice. However, I don't think my approach using the new trait |
| #[pyfunction] | ||
| #[pyo3(text_signature = "(graph, /)")] | ||
| pub fn minimum_cycle_basis(py: Python, graph: &PyGraph) -> PyResult<Vec<Vec<usize>>> { | ||
| py.allow_threads(|| { | ||
| let result = connectivity::minimum_cycle_basis(&graph.graph); | ||
| match result { | ||
| Ok(min_cycle_basis) => { | ||
| // Convert Rust Vec<Vec<NodeIndex>> to Python Vec<Vec<usize>> | ||
| let py_min_cycle_basis = min_cycle_basis | ||
| .iter() | ||
| .map(|cycle| { | ||
| cycle | ||
| .iter() | ||
| .map(|&node_index| graph.graph.to_index(node_index)) | ||
| .collect::<Vec<usize>>() | ||
| }) | ||
| .collect::<Vec<Vec<usize>>>(); | ||
| Ok(py_min_cycle_basis) | ||
| } | ||
| Err(e) => { | ||
| // Handle errors by converting them into Python exceptions | ||
| Err(PyErr::new::<pyo3::exceptions::PyRuntimeError, _>(format!( | ||
| "An error occurred: {:?}", | ||
| e | ||
| ))) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
My piece of advice is the rustworkx-core function looks good but we need to do some work on the Python bindings. I think we can just remove the Python bindings and add it later on
|
I think the code is looking better now, the compiler is complaining about some unused variables + trait missing for Python but overall this was a big improvement! Thanks |
In fact, I do not want to merge my code straightforwardly, because I need some advice!
minimum_cycle_basisand some tests. It works, but I believe it it need some modifications to be more well programmed, including the choice of having error handler or not.minimum_cycle_basisis working fine, we need to add a Python layer to wrap around it. And there are some errors insrc/connectivity/mod.rswhere we use#[pyfunction]annotation. I have no idea to fix this. Perhaps, to fix it, I need to modify even theminimum_cycle_basis.rscore inrustworkx-core/src/connectivity/.Link to the issue https://github.com/Qiskit/rustworkx/issues/1029