Add token swapper to rustworkx-core #765
Conversation
enavarro51
changed the title
Pull Request Test Coverage Report for Build 4731743564
💛 - Coveralls |
|
There is a |
There was a problem hiding this comment.
Overall this LGTM now, thanks for making the updates. I just have a couple small comments in the docs.
The other thing is the API docs for python aren't going to be built, you'll need to add the function to https://github.com/Qiskit/rustworkx/blob/main/docs/source/api.rst to include it in the python documentation.
rustworkx-core/src/token_swapper.rs
Outdated
| if !todo_nodes.is_empty() { | ||
| panic!("got todo nodes"); | ||
| } |
There was a problem hiding this comment.
I assume this panic here is to indicate an internal fault of some kind. I'm just wondering if we either should be doing one of the following here:
- Use
debug_assert!(!todo_nodes.is_empty()) - Have a better error message
- Return a custom Error type
- Use
unreachable!(There were nodes that were not mapped.)
If we expect this to be possible and something a user should recover from I think we should do number 3 and add a custom Error type return. If it's just a sanity check I'd probably opt for 1 so we only do this in the debug build case.
There was a problem hiding this comment.
This was
if todo_nodes:
raise RuntimeError("Too many iterations while approximating the Token Swaps.")
in the python version. I don't know that a user would have any idea what to do with this. I hit it a few times debugging, but it's never come up in final testing. I guess the point is if it happens, you might have an incomplete final swap map. At that point, I don't know what a user could do, except report a bug.
There was a problem hiding this comment.
Hmm, do you want to just change the error message than. How about something like:
| if !todo_nodes.is_empty() { | |
| panic!("got todo nodes"); | |
| } | |
| assert!( | |
| todo_nodes.is_empty(), | |
| "The output final swap map is incomplete, this points to a bug in rustworkx please open an issue." | |
| ); |
That makes it a bit more clear in the code were doing a final sanity check and that it's pointing to an internal bug if there are leftover nodes.
There was a problem hiding this comment.
Done in 0a56fd8. I also changed the one above it for !found to assert.
| @@ -369,6 +369,7 @@ typed API based on the data type. | |||
| rustworkx.graph_complement | |||
| rustworkx.graph_union | |||
| rustworkx.graph_tensor_product | |||
| rustworkx.graph_token_swapper | |||
There was a problem hiding this comment.
Since we don't have a digraph_token_swapper I'm wondering if there is a better section for this. But we can just do this in a followup, it's not critical.
There was a problem hiding this comment.
I vote for let it be for now.
There was a problem hiding this comment.
Thanks for fixing the issue with the index holes (and sorry for my super slow turnaround, the qiskit release has been dominating my time). I left two inline comments which are mutually exclusive. I'm a bit concerned by the performance overhead of trying to recreate the holes the way you did to fix the issue so I suggested a different approach using a mapping to maintain the original indices. If that doesn't work then my other suggestion is just a small allocation tweak to improve the runtime performance slightly.
| // First fill the digraph with nodes. Then since it's a stable graph, | ||
| // must go through and remove nodes that were removed in original graph | ||
| for _ in 0..self.graph.node_bound() { | ||
| digraph.add_node(()); | ||
| } | ||
| let mut count: usize = 0; | ||
| for gnode in self.graph.node_identifiers() { | ||
| let gidx = self.graph.to_index(gnode); | ||
| if gidx != count { | ||
| for idx in count..gidx { | ||
| digraph.remove_node(NodeIndex::new(idx)); | ||
| } | ||
| count = gidx; | ||
| } | ||
| count += 1; | ||
| } |
There was a problem hiding this comment.
Instead of doing this to reproduce the holes exactly what if we maintained a mapping so that we can convert digraph indices to self.graph indices for output. If we changed the type of digraph to StableGraph<G::NodeId, ()> then we can store the original index in the graph when we add nodes. Something like:
for gnode in self.graph.node_identifiers {
digraph.add_node(gnode)
}Then we can just lookup the original index by looking at the weight of the node.
There was a problem hiding this comment.
First reaction is this would be a fairly major restructuring, though I certainly see the efficiency, especially since I might no longer need the NodeIndex to NodeId maps. I did a quick look in other rustworkx code and didn't see anywhere else this was used.
As far as performance is concerned, with the permutations I was using, I got 16 sec for the python token_swapper, 7.5 sec for rust version w/o threads, and 2.5 sec with threads. There was no change in the times when I added the node removal above. Of course, I'm not sure my tests match real life.
I'd vote for leaving this, with a possible future try at restructuring if benchmarking the node removals proves problematic.
There was a problem hiding this comment.
Sure, that's reasonable let's take a look in a follow up. I agree it's not really a runtime concern so much. This method really just adds a single node iteration on top of everything which isn't terrible. I was thinking more about memory efficiency than runtime. BUt we can do this in a follow up.
There was a problem hiding this comment.
LGTM, thanks for your hard work on this feature and sticking with it through all the updates and slow review turnaround (sorry about that)! This will be quite useful both in rustworkx but also qiskit where we're using this function currently to reverse layout permutations in the transpiler with control flow instructions. Having it be significant;y faster will improve performance there, but also having it as a native rust function in rustworkx-core enables us to leverage this functionality in qiskit's rust modules too.
Fixes #701
This PR ports the ApproximateTokenSwapper functionality written in python in qiskit-terra to rustworkx-core. It was based on the paper: Approximation and Hardness for Token Swapping by Miltzow et al. (2016). ArXiV: https://arxiv.org/abs/1602.05150 and work by the qiskit-terra team.
Using a jupyter notebook with the python interface to the rust
token_swapper, the following execution times were noted with a series of 1 to 100 element permutations and creation of swaps to build the inverse permutation,Further rust optimizations may be possible.