-
Notifications
You must be signed in to change notification settings - Fork 145
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
Add token swapper to rustworkx-core #765
Conversation
Pull Request Test Coverage Report for Build 4731743564
💛 - Coveralls |
There is a |
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.
Thanks a bunch for doing this! I'm really looking forward to having this algorithm in rustworkx. I have a few inline comments that I think will clean up the implementation a bit.
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.
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.
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.
Overall I think the code is ready to merge, but I found 2 more small little things inline. Basically I think we need to discuss the panic!
and then one more python side test case which I think would be good to have.
rustworkx-core/src/token_swapper.rs
Outdated
if !todo_nodes.is_empty() { | ||
panic!("got todo nodes"); | ||
} |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for let it be for now.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.