-
Notifications
You must be signed in to change notification settings - Fork 160
Migrate topological codes fitters to use retworkx #552
Migrate topological codes fitters to use retworkx #552
Conversation
This commit migrates the networkx usage in the topological codes fitters module away from networkx to use retworkx instead. This should provide a speedup as retworkx is typically much faster than networkx. But, more importantly this module is the last usage of networkx in qiksit and that causes noticeable import overhead as networkx is slow to import. Moving to retworkx like the rest of Qiskit will remove this overhead.
@quantumjim I'm having a hard time debugging the failures I'm hitting here. retworkx behaves a bit differently than networkx nodes are integer indexed (like an array except there can be holes in case of node removals) and the networkx code in the fitters depends heavily on the associative nature of networkx (access nodes by the payload). If you have a simpler or more deterministic test case than what's in the unit tests that would be really helpful. |
ok, now I'm really confused the one topological codes test passes on the linux 3.6 and 3.7 job: https://github.com/Qiskit/qiskit-ignis/pull/552/checks?check_run_id=1787089496#step:6:571 and https://github.com/Qiskit/qiskit-ignis/pull/552/checks?check_run_id=1787089459#step:6:609 But it's failing reliably on Python 3.8 locally for me and in CI on python 3.8 for macOS: https://github.com/Qiskit/qiskit-ignis/pull/552/checks?check_run_id=1787089507#step:6:653 There shouldn't be any environment difference like this in the changes here since nothing should be dependent on a specific python version or OS in the diff here or on the retworkx side. |
I'm guessing that the The matching problem that needs to be solved here is finding a minimum weight matching that must be perfect, but only on a certain subset of nodes. It was done by using the The I'd like to find some more specific examples, but I had trouble installing your fork of Ignis (and the corresponding version of retworkx) so I can't check for cases of bad matchings myself. But just to suggest something, how does it deal with a case like the following?
|
Yeah I think you're right. I managed to find some cases where the retworkx max_weight_matching is differing from the networkx result (and I assume networkx is correct). I added more tests to the PR (see: Qiskit/rustworkx@0c6ea26 ) that fail I'll work on fixing that in the PR and rerun things here when I have a fix (it might be a while, that algorithm is not simple). As for testing this locally it shouldn't be too bad to install. For retworkx you just need to have the rust compiler installed on your local system (rustup makes this easy: https://rustup.rs/) then you can run: I did run your example locally and it does fail, the |
@quantumjim ok, I've got this to the point where your example script is passing locally now. There was a bug in getting the distance from the syndrome graph and putting it into the error graph. However the unit tests are still failing (although it seems like a nondeterministic failure, I can get it to pass some of the time). I don't think it's an issue Qiskit/rustworkx#229 anymore, mostly because I ramped up the testing in that PR even more so it's testing random graphs against networkx's output and it's the same most of the time (and when it's not there are multiple maximum weight matchings and the retworkx code is still picking a valid matching). |
It's giving out error rates of around 0.05 that should be more like 0.001. So there is definitely something wrong. I'd guess that the passes are lucky coincidences. For a test that's deterministic and more specific, I'd suggest trying out all single Pauli errors individually to verify that they are corrected with certainty. Here's a script that does that |
Thanks that script was helpful and I figured out my bug. I forgot to tell retworkx how to get an integer weight from an edge, so it was incorrectly defaulting all the edge weights to 1. I added the missing |
retworkx 0.8.0 was released earlier today that included the |
Summary
This commit migrates the networkx usage in the topological codes fitters
module away from networkx to use retworkx instead. This should provide
a speedup as retworkx is typically much faster than networkx. But, more
importantly this module is the last usage of networkx in qiskit and that
causes noticeable import overhead as networkx is slow to import. Moving
to retworkx like the rest of Qiskit will remove this overhead.
Details and comments
TODO:
max_weight_matching
)max_weight_matching
(Add max weight matching function Qiskit/rustworkx#229)rx.PyGraph(multigraph=False)
instead of tracking edges manually with a set