Implement $O(n^3)$ flow finding algorithm#294
Implement $O(n^3)$ flow finding algorithm#294Newtech66 wants to merge 13 commits intoTeamGraphix:masterfrom
Conversation
|
@Newtech66 (CC: @thierry-martinez @shinich1 ) Thank you for opening this PR.
|
|
Hi @EarlMilktea , we will comment on this PR soon with @thierry-martinez. Note that there is also a Since this is a Python implementation, I think benchmarking it against Python implementations makes more sense. Also, @Newtech66 saw a convention issue in the old flow-finding algorithms and we are currently investigating why this has not been seen. Another suggestion, maybe don't replace the old implementation so that we can compare the two versions. We'll update you soon. In any case thanks for the work! |
I'm strongly opposed to this idea for code quality improvements. It must be replaced, at least after merging this PR. Anyway keep me updated! |
EarlMilktea
left a comment
EarlMilktea
left a comment
There was a problem hiding this comment.
MEMO: Reviews work in progress.
graphix/gflow.py
Outdated
| non_output_idx = np.searchsorted(nodes, non_output_nodes) | ||
| non_input_map = {v: i for v, i in zip(non_input_nodes, non_input_idx)} | ||
| non_output_map = {v: i for v, i in zip(non_output_nodes, non_output_idx)} | ||
| n, nI, nO = len(nodes), len(iset), len(oset) |
There was a problem hiding this comment.
Please respect PEP8 naming convention (ni, no, etc,)
|
@Newtech66 (CC: @shinich1 ) Opened TeamGraphix/swiflow#9 to collaborate with this PR in the future. |
|
This comment has to be addressed to proceed with the tests for this PR. |
|
At the maintainer meeting, we came to the following conclusions:
|
|
Hi @Newtech66 Thanks! |
|
Additional comments from me, mainly related to maintainability:
|
You're right, I planned to do that once I was done writing the function, but I just forgot somehow... |
|
All the tests are passing now. |
|
@Newtech66 could you resolve conflict with master branch, so that CI can run? |
|
@shinich1 Done. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #294 +/- ##
==========================================
+ Coverage 79.83% 80.47% +0.64%
==========================================
Files 36 36
Lines 5429 5414 -15
==========================================
+ Hits 4334 4357 +23
+ Misses 1095 1057 -38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@shinich1 @Newtech66 @mgarnier59 @thierry-martinez My suggestion: why not merge this PR (of course, after ready) at the same time with #280 ? This is because
|
|
Indeed, if there's no urgency to merge this PR (i.e., if we're out of time to merge it before the Unitary Hack), then it's better to wait for #280 to be merged first. |
|
There's only 8.5 hours before the end of Unitary Hack. |
8.5 hours is obviously too short for reviewing this PR, but at the same time I personally feel @Newtech66 's effort should somehow be appreciated. |
|
Hack allows the PR to be open until the 16th. I will assign @Newtech66 to issue so that it's clear you're still working on the bountied issue (to the UF). |
|
@EarlMilktea Actually, why don't we port this new algorithm over to |
There're some good reasons for not doing so.
It should be considered as the last resort only for performance-critical fractions.
|
|
Any updates on this? |
My understanding is that we tried but are still struggling to finalize our design. |
|
#337 subsumes this PR. |
Introduce the `O(N^3)` Pauli flow-finding algorithm presented in [1] which improves the previous version with `O(N^5)` complexity [2]. Since a Pauli flow in an open graph without Pauli measurements defines a gflow, this algorithm also improves the previous implementation of the gflow-finding algorithm with `O(N^4)` complexity [3]. The implementation proposed here considers the suggestion in [1] to eliminate the rows that are identically 0 in the order-demand matrix to accelerate the flow-finding routine in open graphs with nI ≠ n O. Finally, the new algorithm solves the issue discussed in qat-inria#17 , whereby the previous flow-finding functions could return a correction function containing input nodes in the codomain, which is not allowed by the definition of gflow or Pauli flow. In summary: - The core functions of the algorithm are encapsulated in the new module `graphix.find_pflow`, while `graphix.gflow` and related modules are adapted accordingly to avoid modifying the current API in this PR. - The module `graphix.linalg` has been renamed `graphix._linalg` and updated with more efficient functions for gaussian elimination, back substitution, kernel-finding and right-inverse. New implementation is GF2-specialized and, where appropriate, just-in-time compiled with numba. The dependence on `sympy` and `galois` is dropped. Tests are simplified and updated accordingly, and module is now typed. - `pytest-benchmark` is incorporated to allow for easier benchmarking between different commits (courtesy of @thierry-martinez). Additional comments --------------------- - This PR addresses the same issue as #294, but is otherwise unrelated. - The module `graphix.gflow` will be restructured in a future PR. The implementation exhibits the cubic scaling as described in [1]. See https://gitlab.inria.fr/muldemol/pflow-on3-scaling.git for a benchmarking code. References ----------- [1] Mitosek and Backens, 2024 (arXiv:2410.23439) [2] Simmons, 2021 (arXiv:2109.05654) [3] Backens et al., Quantum 5, 421 (2021) --------- Co-authored-by: Thierry Martinez <thierry.martinez@inria.fr>
5 participants
Description of the change:
modeargument of the old flow finding algorithm in the new one.right_inverse()tolinalg.py.GF2.row_reduce.Fix incorrect(fixed in PR Fix handling of plane YZ in Pauli flow #297)get_pauli_nodesmethod.Related issue:
Closes #279.
Checks:
nox)ruffCONTRIBUTING.mdfor more details