Improved MCXVChain with dirty auxiliary qubits #9687

rafaella-vale · 2023-03-01T03:40:06Z

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.

qiskit-bot · 2023-03-01T03:40:10Z

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:

@Cryoris
@Qiskit/terra-core
@ajavadia

coveralls · 2023-03-03T02:52:29Z

Pull Request Test Coverage Report for Build 4333363738

51 of 52 (98.08%) changed or added relevant lines in 1 file are covered.
168 unchanged lines in 12 files lost coverage.
Overall coverage increased (+0.02%) to 85.338%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/library/standard_gates/x.py	51	52	98.08%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passmanager_config.py	1	97.18%
src/vf2_layout.rs	1	86.44%
qiskit/circuit/bit.py	2	87.5%
qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py	2	98.08%
qiskit/pulse/library/waveform.py	3	91.67%
qiskit/quantum_info/synthesis/one_qubit_decompose.py	7	93.86%
qiskit/transpiler/instruction_durations.py	7	91.18%
qiskit/visualization/counts_visualization.py	12	79.87%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	14	95.04%
qiskit/visualization/circuit/circuit_visualization.py	16	69.64%

Totals
Change from base Build 4318901192:	0.02%
Covered Lines:	67922
Relevant Lines:	79592

💛 - Coveralls

adjs · 2023-03-06T18:17:32Z

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.

Cryoris

The overall code LGTM, thanks for the improvement! I left some comments below.

qiskit/circuit/library/standard_gates/x.py

Cryoris · 2023-03-09T08:52:31Z

qiskit/circuit/library/standard_gates/x.py

+            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


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? 🙂

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.

When calling the private function on the qiskit.synthesis we must create a new file in a new directory?

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.

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.

Cryoris · 2023-03-09T08:54:01Z

qiskit/circuit/library/standard_gates/x.py

+                                            [],
+                                        )
+                                    )
+                                    definition.append((U1Gate(pi / 4), [targets[i]], []))  # T gate


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?

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.

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.

ShellyGarion · 2024-02-28T07:48:29Z

@adjs @rafaella-vale - Following the recent discussion in #5872 I'm checking on the status of this PR.
I believe that we are still waiting for a recent version following the @Cryoris review comments.
If you still need any help from us, please let us know.

ShellyGarion · 2024-03-04T07:20:09Z

Could you please merge your branch with the main branch, fix the conflicts and the lint and docs errors?

coveralls · 2024-03-13T14:57:27Z

Pull Request Test Coverage Report for Build 9702834241

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

54 of 55 (98.18%) changed or added relevant lines in 1 file are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.02%) to 89.782%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/library/standard_gates/x.py	54	55	98.18%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	4	93.38%

Totals
Change from base Build 9698604147:	0.02%
Covered Lines:	63877
Relevant Lines:	71147

💛 - Coveralls

ShellyGarion · 2024-03-13T15:07:20Z

Could you please rebase your code on the current main branch of Qiskit and remove all redundant PRs?
It's impossible to review the code this way (there are over 200 files :) )

ShellyGarion · 2024-03-13T16:15:15Z

test/python/circuit/test_controlled_gate.py

@@ -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


I don't think this line is needed. It duplicates with line 87.
In addition, could you please handle the black errors?

IsmaelCesar · 2024-03-13T16:23:18Z

Could you please merge your branch with the main branch, fix the conflicts and the lint and docs errors?

We have been able to fix the errors and replace the U1/2 operators by T and H operators directly.
In our implementation we use circuit.append(operator_class, [qubit_indices], []) because we are following the same coding stile present in the file qiskit/circuit/library/standard_gates/x.py , where instead of using circuit.operation(qubit_index), the lines present the append function with lists of operator classes and qubit indices, that is why we decided to use a similar approach when implementing the modifications.

IsmaelCesar · 2024-03-13T16:29:52Z

Could you please rebase your code on the current main branch of Qiskit and remove all redundant PRs? It's impossible to review the code this way (there are over 200 files :) )

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.

ShellyGarion

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.

ShellyGarion · 2024-03-17T13:16:13Z

qiskit/circuit/library/standard_gates/x.py

@@ -1416,6 +1418,8 @@ def __init__(
        duration=None,
        unit="dt",
        _base_label=None,
+        relative_phase: bool = False,
+        action_only: bool = False,


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?

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.

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.

ShellyGarion · 2024-03-17T13:26:42Z

qiskit/circuit/library/standard_gates/x.py

+                                    qc.cx(q_controls[self.num_ctrl_qubits - i - 1], targets[i])
+                                    qc.tdg(targets[i])
+                                    qc.h(targets[i])
+                                else:


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?

ShellyGarion · 2024-03-31T07:46:08Z

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

Cryoris

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!

ShellyGarion

LGTM. We very much appreciate your contribution to Qiskit!
@adjs @rafaella-vale @IsmaelCesar @israelferrazaraujo @thiagom123

ShellyGarion · 2024-07-07T06:35:17Z

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>

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Mar 1, 2023

rafaella-vale mentioned this pull request Mar 1, 2023

Fixes issue #9741 from exponential to linear number of cx gates in mcry #9688

Merged

ShellyGarion mentioned this pull request Mar 2, 2023

[WIP] restructuring MCX synthesis code #7505

Closed

adjs force-pushed the mcx_vchain_dirty branch from fa32cab to 837b23b Compare March 3, 2023 00:55

adjs mentioned this pull request Mar 6, 2023

Improving MCXRecursive #8710

Closed

rafaella-vale marked this pull request as ready for review March 7, 2023 14:35

rafaella-vale requested a review from a team as a code owner March 7, 2023 14:35

rafaella-vale changed the title ~~[WIP] Improved MCXVChain with dirty auxiliary qubits~~ Improved MCXVChain with dirty auxiliary qubits Mar 7, 2023

adjs mentioned this pull request Mar 8, 2023

Decompose CnXGate grows exponentially in terms of CX, instead of linear #5872

Closed

Cryoris reviewed Mar 9, 2023

View reviewed changes

IsmaelCesar force-pushed the mcx_vchain_dirty branch from d373933 to 0f7730e Compare February 29, 2024 14:13

rafaella-vale requested review from a team, alexanderivrii and jyu00 as code owners March 13, 2024 14:40

IsmaelCesar force-pushed the mcx_vchain_dirty branch from a4a11c8 to 0f7730e Compare March 13, 2024 15:21

ShellyGarion removed request for a team and jyu00 March 13, 2024 15:32

ShellyGarion reviewed Mar 13, 2024

View reviewed changes

ShellyGarion requested changes Mar 17, 2024

View reviewed changes

improved mcxvchain with dirty aux qubits

087d986

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>

rafaella-vale and others added 14 commits June 27, 2024 16:28

black

bd76eeb

lint

5f2c6f7

Update x.py

f5e2c3a

Update test_controlled_gate.py

ab6a4c4

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

41d0db9

release notes

e7ed2c1

removing non existend module

e460a83

removing U1 and U2 gates MCXVChain

13fd6b0

black

013c8a5

using the quantum_circuit.operation(qubit_indices) notation

0480af4

mcx

0f1c253

comments

e61eda3

lint

17da83e

IsmaelCesar force-pushed the mcx_vchain_dirty branch from 4955c5c to 17da83e Compare June 27, 2024 19:46

rearranging transpile import

95264ed

Cryoris approved these changes Jul 5, 2024

View reviewed changes

ShellyGarion added mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library Changelog: New Feature Include in the "Added" section of the changelog labels Jul 7, 2024

ShellyGarion approved these changes Jul 7, 2024

View reviewed changes

ShellyGarion added this pull request to the merge queue Jul 7, 2024

ShellyGarion added this to the 1.2.0 milestone Jul 7, 2024

Merged via the queue into Qiskit:main with commit ee41b18 Jul 7, 2024
15 checks passed

ShellyGarion mentioned this pull request Jul 7, 2024

MCXVChain with dirty ancilla is not optimal. #9740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved MCXVChain with dirty auxiliary qubits #9687

Improved MCXVChain with dirty auxiliary qubits #9687

rafaella-vale commented Mar 1, 2023 •

edited

Loading

qiskit-bot commented Mar 1, 2023

coveralls commented Mar 3, 2023 •

edited

Loading

adjs commented Mar 6, 2023

Cryoris left a comment

Cryoris Mar 9, 2023

ShellyGarion Mar 9, 2023 •

edited

Loading

IsmaelCesar Mar 14, 2024

ShellyGarion Mar 14, 2024 •

edited

Loading

ShellyGarion Mar 14, 2024

Cryoris Mar 9, 2023

rafaella-vale Mar 11, 2023

Cryoris Feb 28, 2024

ShellyGarion commented Feb 28, 2024

ShellyGarion commented Mar 4, 2024

coveralls commented Mar 13, 2024 •

edited

Loading

ShellyGarion commented Mar 13, 2024

ShellyGarion Mar 13, 2024

IsmaelCesar commented Mar 13, 2024

IsmaelCesar commented Mar 13, 2024

ShellyGarion left a comment

ShellyGarion Mar 17, 2024

ShellyGarion Mar 17, 2024

ShellyGarion Mar 17, 2024

ShellyGarion Mar 17, 2024

ShellyGarion commented Mar 31, 2024

Cryoris left a comment

ShellyGarion left a comment

ShellyGarion commented Jul 7, 2024

Improved MCXVChain with dirty auxiliary qubits #9687

Improved MCXVChain with dirty auxiliary qubits #9687

Conversation

rafaella-vale commented Mar 1, 2023 • edited Loading

Summary

Details and comments

qiskit-bot commented Mar 1, 2023

coveralls commented Mar 3, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4333363738

💛 - Coveralls

adjs commented Mar 6, 2023

Cryoris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyGarion Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyGarion Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyGarion commented Feb 28, 2024

ShellyGarion commented Mar 4, 2024

coveralls commented Mar 13, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9702834241

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

ShellyGarion commented Mar 13, 2024

Choose a reason for hiding this comment

IsmaelCesar commented Mar 13, 2024

IsmaelCesar commented Mar 13, 2024

ShellyGarion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyGarion commented Mar 31, 2024

Cryoris left a comment

Choose a reason for hiding this comment

ShellyGarion left a comment

Choose a reason for hiding this comment

ShellyGarion commented Jul 7, 2024

rafaella-vale commented Mar 1, 2023 •

edited

Loading

coveralls commented Mar 3, 2023 •

edited

Loading

ShellyGarion Mar 9, 2023 •

edited

Loading

ShellyGarion Mar 14, 2024 •

edited

Loading

coveralls commented Mar 13, 2024 •

edited

Loading