Skip to content
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

Implementation of the Gray-Synth and Patel–Markov–Hayes algorithms #2457

Merged
merged 32 commits into from
Jul 1, 2019

Conversation

BoschSamuel
Copy link
Contributor

Summary

I added an implementation of the Gray-Synth algorithm [1] and the Patel–Markov–Hayes algorithm [2]

Details and comments

Hello again,
my name is Samuel Bosch, and I am a master's student at EPFL in Switzerland. I work in the group of Prof. Giovanni de Micheli on Quantum Computing. My supervisors are Dr. Mathias Soeken and Bruno Schmitt. I started programming in Qiskit quite recently and would like to make my second contribution to Qiskit-Terra.

The implementation of the Gray-Synth algorithm is explained in detail in reference [1] in section 4. The code of the algorithm is based on the pseudo-code named "Algorithm 1. Algorithm for synthesizing a parity network".

The implementation of the Patel–Markov–Hayes algorithm (which is needed in the Gray-Synth algorithm) can be found in reference [2]. The code of the algorithm is based on the pseudo-code named "Algorithm 1: C-NOT Circuit Synthesis".

I wasn't really sure where to put my code, so I put it under "qiskit-terra⁩ ▸ ⁨qiskit⁩ ▸ ⁨transpiler⁩ ▸ ⁨passes⁩", which might not be the optimal location. Let me know if there is anything I can change in the code.

Best regards,
Samuel

[1] Amy, Matthew, Parsiad Azimzadeh, and Michele Mosca. "On the controlled-NOT complexity of controlled-NOT–phase circuits." Quantum Science and Technology 4.1 (2018): 015002.

[2] Patel, Ketan N., Igor L. Markov, and John P. Hayes. "Optimal synthesis of linear reversible circuits." Quantum Information & Computation 8.3 (2008): 282-294.

@BoschSamuel
Copy link
Contributor Author

I am receiving the following error and don't know what to do about it:
test/python/transpiler/test_graysynth.py:89:8: E1101: Instance of 'TestGraySynth' has no 'assertTrue' member (no-member)
Could someone help me out with this? Thank you!

@ajavadia
Copy link
Member

Hi @BoschSamuel, thank you for your recent contributions! We will try to review them as soon as possible.

About the error you are getting here, you have to do this:

from qiskit.test import QiskitTestCase

 class TestGraySynth(QiskitTestCase):
    ...

@BoschSamuel
Copy link
Contributor Author

I am also not sure why the "cyclic-import" error occurs. When I run make style && make lint on my computer, this doesn't happen.

Copy link
Member

@ajavadia ajavadia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a quick look and left some comments.
This file should not be in the transpiler/passes directory, because it is not a pass. Passes take a DAGCircuit and return a DAGCircuit (their classes inherit from AnalysisPass or TransformationPass).
I think we need a synthesis directory in the qiskit/transpiler folder, dedicated to different synthesis algorithms. Would you like to create this? We should move statevector synthesis and unitary synthesis algorithms to it as well.

qiskit/transpiler/passes/graysynth.py Outdated Show resolved Hide resolved
qiskit/transpiler/passes/graysynth.py Outdated Show resolved Hide resolved
qiskit/transpiler/passes/graysynth.py Outdated Show resolved Hide resolved
qiskit/transpiler/passes/graysynth.py Outdated Show resolved Hide resolved
qiskit/transpiler/passes/graysynth.py Outdated Show resolved Hide resolved
qiskit/transpiler/passes/graysynth.py Outdated Show resolved Hide resolved
@ajavadia ajavadia self-assigned this Jun 17, 2019
@ajavadia
Copy link
Member

@boschmitt I had to read the GraySynth and Patel-Markov-Hayes algorithms in detail in order to review this, so it took me a bit of time. But I think it is ready now.

I made some changes. The commit history should be descriptive, but here is the gist:

  • added a couple more tests, mostly using the examples in the original papers
  • in adding tests, I found a bug in the cnot_synth algorithm, which I fixed in 9762a50
  • I made some API changes for user-friendliness. In particular, I made the cnot_synth method standalone and not just an extension of graysynth. I also removed some unnecessary args (like number, which could just be inferred), and gave a default of 2 to the n_sections arg.
  • some docstring/cosmetic updates

return circ


def _lwr_cnot_synth(state, n_sections):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the n_sections argument should actually be section_size, because that is how it is used in the paper, and that is how you seem to be interpreting it in line 259.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree!

cnots_copy = np.transpose(np.array(copy.deepcopy(cnots)))
state = np.eye(n_qubits).astype('int') # This matrix keeps track of the state in the algorithm

# Check if some T-gates can be applied, before adding any C-NOT gates
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite follow this part of the code, what is the benefit of adding the phase gates early?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of this part of the code is just to apply those phase-shift gates before the actual algorithms starts (it should say phase-shift gates, not T-gates...I fixed this now). It is just there to speed things up, as the whole algorithm then doesn't have to deal with such trivial gates

@ajavadia
Copy link
Member

One thing I wasn't able to reproduce from the paper is the example of doubly-controlled Z rotation in Figure 3.

A 6-cnot construction should be found according to the paper:
image

but here a 7-cnot construction is found:
image

Code for the above:

from qiskit.transpiler.synthesis import graysynth
cnots = [[1, 0, 1, 0, 1, 1, 0],
         [0, 1, 0, 1, 1, 1, 0],
         [0, 0, 1, 1, 1, 0, 1]]
angles = [1/8, 1/8, 3/8, 3/8, 1/8, 3/8, 1/8]
c_gray = graysynth(cnots, angles, 3)
c_gray.draw(output='mpl')

@BoschSamuel
Copy link
Contributor Author

@ajavadia Thank you for the detailed review. I will work on this as soon as possible, as things are a bit crazy for me this and next week :)

@ajavadia
Copy link
Member

ajavadia commented Jul 1, 2019

This looks good to me now, thanks for the effort! I'll open an issue about that double-controlled Z synthesis to be investigated further.

@ajavadia ajavadia merged commit 8d80735 into Qiskit:master Jul 1, 2019
lia-approves pushed a commit to edasgupta/qiskit-terra that referenced this pull request Jul 30, 2019
…iskit#2457)

* implemented Gray-Synth algorithm

* fixed formatting of the code

* added functionality for processing matrices with same columns (multiple gates applied to same state)

* added functionality for processing matrices with same columns (multiple gates applied to same state) also during the algorithm

* fixed 2 bugs in lwr_cnot_synth() and in graysynth()

* minor formatting fixes

* Created test_graysynth.py file, for checking if graysynth.py works properly

* minor improvements

* fixed error as explained by @ajavadia

* changed __init__.py

* Fixed cyclic import error (suggested by @ajavadia)

Co-Authored-By: Ali Javadi-Abhari <[email protected]>

* Created new folder transpiler/synthesis for synthesis algorithms and put GraySynth algorithm into it. More changes to follow

* upated testing function accordingly

* added function documentations, removed redundant state = np.matrix(state) line

* removed storage class, and replaced it with regular list using its stack functionality, and added raise Exception for numpy.ndarray

* added documentation for Exception in function cnot_synth()

* instead of using T-gates by default, it is now possible to specify any arbitrary phase-gate rotation

* changelog.md

* type fix angels -> angles

* docstring edits

* remove qreg from cnot_synth args

* remove number (n_qubits) from function args as it can be inferred

* specify a default of 2 for n_sections

* simplify test via Operator class, and add test from paper example

* make cnot_synth standalone (not cnot appender), and use QiskitError

* fix bug in cnot_synth and add test

* cnot_synth takes python lists or numpy ndarray

* lint

* Update graysynth.py

* renamed n_sections --> section_size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants