-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Improved MCXVChain with dirty auxiliary qubits #9687
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this: |
Pull Request Test Coverage Report for Build 4333363738
💛 - Coveralls |
This PR solves issue #9740 . from qiskit import transpile
from qiskit.circuit.library.standard_gates import MCXVChain
k = 10
qc = MCXVChain(k, True).definition
tqc = transpile(qc, basis_gates=['u', 'cx'])
tqc.count_ops()['cx'] Now the required number of cx gates is 8k-6. PS. This PR was a task for the students of the quantum computing class at Centro de Informática - UFPE. |
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.
The overall code LGTM, thanks for the improvement! I left some comments below.
elif not self._relative_phase and self.num_ctrl_qubits == 3: | ||
definition.append((C3XGate(), [*q_controls, q_target], [])) | ||
else: | ||
num_ancillas = self.num_ctrl_qubits - 2 |
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.
To make this code a little easier to read and make it easier to potentially move the code to qiskit.synthesis
at a later stage, could you put this synthesis technique into a private function (could be in this file) and then just call that function here? 🙂
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.
In addition, it would be useful to keep the current synthesis code in another internal function.
Namely, to put the current synthesis code in the definition in a function called _synth_MCXVChain_basic
and the new code in _synth_MCXVChain_isometry
. Both functions should return a QuantumCircuit
. The definition will call the new synthesis function.
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.
When calling the private function on the qiskit.synthesis
we must create a new file in a new directory?
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.
In the meanwhile, you don't need to move the code to qiskit.synthesis
. Just organize it in internal functions as we suggested above (in this file), so that later it will be easier for us to move it if needed.
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.
The idea is that not all this code will be inside the define
function.
You can put the existing code inside _synth_MCXVChain_basic
and the new code in _synth_MCXVChain_isometry
,
and let the define
function call the new _synth_MCXVChain_isometry
.
[], | ||
) | ||
) | ||
definition.append((U1Gate(pi / 4), [targets[i]], [])) # T gate |
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.
In the paper by Iten you're referencing, the decomposition of Lemma 8 is using an Ry(pi/4) gate (what they call A), but you're using single T gates sometimes -- could you briefly explain why this is equivalent here?
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.
Both result in a Toffoli gate up to some phase. The gate A could be a different gate as long as the circuit is its own inverse. We were trying to keep similarities with Qiskit's code and used the same single-qubit gates from the RCCX circuit, but we could change them to Ry gates.
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.
Using a T gate (or any RZ-like gate) is better for now as that can be implemented without a physical pulse on the device. Though that probably doesn't matter so much as these MCX decompositions are likely more interesting for fault-tolerance anyways 😄 Could you use a TGate or PhaseGate instead of U1
though? The latter is legacy IBM specific.
@adjs @rafaella-vale - Following the recent discussion in #5872 I'm checking on the status of this PR. |
d373933
to
0f7730e
Compare
Could you please merge your branch with the main branch, fix the conflicts and the lint and docs errors? |
Pull Request Test Coverage Report for Build 9702834241Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Could you please rebase your code on the current main branch of Qiskit and remove all redundant PRs? |
a4a11c8
to
0f7730e
Compare
@@ -19,7 +19,8 @@ | |||
from numpy import pi | |||
from ddt import ddt, data, unpack | |||
|
|||
from qiskit import QuantumRegister, QuantumCircuit, QiskitError | |||
from qiskit import QuantumRegister, QuantumCircuit, QiskitError, transpile | |||
from qiskit.test import QiskitTestCase |
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 don't think this line is needed. It duplicates with line 87.
In addition, could you please handle the black errors?
|
I apologize for the confusion. I had a small problem while rebasing the code. Fortunately I was able to undo the problem and reapply the changes. |
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 @rafaella-vale @IsmaelCesar @adjs for your contribution.
The code looks well, and I only have some comments on the API docs and code documentation.
@@ -1416,6 +1418,8 @@ def __init__( | |||
duration=None, | |||
unit="dt", | |||
_base_label=None, | |||
relative_phase: bool = False, | |||
action_only: bool = False, |
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.
Could you please add proper documentation on all of these parameters so that the user will know what they are doing and what to choose?
It will be good to extend the docstring of the MCXVChain
class, add a reference to the relevant paper and theorem.
Perhaps even some examples if possible?
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.
It seems that there are 3 boolean parameters:
dirty_ancillas, relative_phase, action_only
Could we have all 8 options for these parameters? or only part of the options are possible?
If so, please add it to the docs, check and raise a QiskitError
if the user chose a bad option.
Otherwise, please add tests for all relevant options.
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.
Are there some bounds on the number of CX
gates for these options?
If so, could you please add this to the docs, as well as add proper tests.
qc.cx(q_controls[self.num_ctrl_qubits - i - 1], targets[i]) | ||
qc.tdg(targets[i]) | ||
qc.h(targets[i]) | ||
else: |
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 code looks much clearer now, but it seems that it contains many if/else
clauses.
Could you please add a comment after each if/else/elif
and to say which case it handles?
This PR is almost ready to go. It would be nice to finalize it (as well as #8710) for the Qiskit 1.1 release |
Co-authored-by: thiagom123 <thiagomdazevedo@hotmail.com> Co-authored-by: IsmaelCesar <leamscesar@gmail.com> Co-authored-by: Israel F. Araujo <israelferrazaraujo@hotmail.com> Co-authored-by: Adenilton Silva <7927558+adjs@users.noreply.github.com>
Co-authored-by: thiagom123 <thiagomdazevedo@hotmail.com> Co-authored-by: IsmaelCesar <leamscesar@gmail.com> Co-authored-by: Israel F. Araujo <israelferrazaraujo@hotmail.com> Co-Authored-By: Adenilton Silva <7927558+adjs@users.noreply.github.com>
fix test_mcxvchain_dirty_ancilla_relative_phase reduce number of controls in test_mcxvchain_dirty_ancilla_action_only and test_mcxvchain_dirty_ancilla_relative_phase
4955c5c
to
17da83e
Compare
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 all the effort of implementing the better algorithm! Some things like docstrings can maybe be tuned a bit more, but let's tackle that in a follow up with the Rust port or the circuit library refactor and get this improvement in Qiskit.
I'm not putting this in merge queue so @ShellyGarion can have another final look!
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. We very much appreciate your contribution to Qiskit!
@adjs @rafaella-vale @IsmaelCesar @israelferrazaraujo @thiagom123
close #9740 |
* improved mcxvchain with dirty aux qubits Co-authored-by: thiagom123 <thiagomdazevedo@hotmail.com> Co-authored-by: IsmaelCesar <leamscesar@gmail.com> Co-authored-by: Israel F. Araujo <israelferrazaraujo@hotmail.com> Co-authored-by: Adenilton Silva <7927558+adjs@users.noreply.github.com> * tests for improved mcxvchain with dirty aux qubits Co-authored-by: thiagom123 <thiagomdazevedo@hotmail.com> Co-authored-by: IsmaelCesar <leamscesar@gmail.com> Co-authored-by: Israel F. Araujo <israelferrazaraujo@hotmail.com> Co-Authored-By: Adenilton Silva <7927558+adjs@users.noreply.github.com> * black * lint * Update x.py * Update test_controlled_gate.py fix test_mcxvchain_dirty_ancilla_relative_phase reduce number of controls in test_mcxvchain_dirty_ancilla_action_only and test_mcxvchain_dirty_ancilla_relative_phase * Update test_controlled_gate.py * release notes * removing non existend module * removing U1 and U2 gates MCXVChain * black * using the quantum_circuit.operation(qubit_indices) notation * mcx * comments * lint * rearranging transpile import --------- Co-authored-by: thiagom123 <thiagomdazevedo@hotmail.com> Co-authored-by: IsmaelCesar <leamscesar@gmail.com> Co-authored-by: Israel F. Araujo <israelferrazaraujo@hotmail.com> Co-authored-by: Adenilton Silva <7927558+adjs@users.noreply.github.com>
Summary
Implementation of MCX with$k$ controls and $k - 2$ dirty auxiliary qubits following the optimizations described by Iten et al. in [1].
[1] http://arxiv.org/abs/1501.06911
Details and comments
The changes made to the MCXVChain class only affect the dirty_ancilla mode. This implementation produces MCX circuits with at most$8k - 6$ CNOTs with $k$ control qubits. It also addresses issue #5872 and is a necessary step to further improve PR #8710.