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

Inverse QFT - Implementation and Test #15

Merged
merged 4 commits into from
Jan 30, 2017
Merged

Inverse QFT - Implementation and Test #15

merged 4 commits into from
Jan 30, 2017

Conversation

vontell
Copy link
Contributor

@vontell vontell commented Jan 29, 2017

I was looking to easily add the inverse QFT to an instruction set, but saw that it was not implemented. Since (I believe, there is surprisingly little literature on the inverse implementation) it is simply the QFT in reverse (with negative angles), I have added it as another method in the fourier.py file. I also added a simple test which takes a bit string, applies the QFT followed by the inverse QFT, and verifies that the final result equals the initial result, but note that this relies on the correctness of the QFT (which does not have tests yet as far as I can see).

@willzeng willzeng self-requested a review January 29, 2017 03:40
Copy link
Contributor

@willzeng willzeng left a comment

Choose a reason for hiding this comment

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

Hi @vontell thanks for this. The inverse QFT looks great. Something we're excited to add in the roadmap are some general features in pyQuil that will allow you to take the dagger-adjoint of programs directly, though having this directly written out in the meantime works for me.

A general comment on the testing setup is that on client libraries like grove and pyquil our modus operandi is to avoid directly running against the forest API in testing. Instead would suggest to either:

  1. mock out the qvm connection (an example using patch is in test_param_prog_p1_barbell of the file /grove/pyqaoa/tests/test_maxcut.py, or
  2. test your inverse function against the program that is generated to ensure correctness there

Unfortunately it does mean that there isn't any easy way to test the invariant that this and the qft are actually inverses of each other, but right now we want to keep the tests modular so that people without keys can still run tests successfully.

Other suggestions for the testing welcome @tarballs-are-good @ncrubin

@ncrubin
Copy link
Contributor

ncrubin commented Jan 30, 2017

Hey Aaron, Thanks for the contribution. We can further modularize to reduce the repeated core_(inverse_)qft function. I suggest making an internal method, denoted with a leading underscore, that performs the recursive construction of the qft program. Your inverse function and the original qft method could call the _core_qft() to generate the appropriate circuit.

New methods would be:

qft()

_core_qft()

inverse_qft().

To simplify the inverse operation you could have _core_qft() take the qubits and a coeff=[1, -1]. That way you don't have to parse the gate parameter and negate the angle in the inverse_qft() method.

As an example:

def qft():
       p = pq.Program().inst(core_qft(qubits, 1))
      return p + bit_reversal(qubits)

@vontell
Copy link
Contributor Author

vontell commented Jan 30, 2017

Another way this could be cleaned up is with a modification to the pop() method in the InstructionGroup class; currently it pops an instruction but does not return it, so the inverse_qft() function has to directly reference the actions list. Returning the popped instruction would also be helpful for validation purposes (if someone wants to confirm what instruction is popped off).

@willzeng
Copy link
Contributor

willzeng commented Jan 30, 2017

I like this suggestion that .pop() return the instruction and have added it as an issue to the pyquil repo

@stylewarning stylewarning requested review from stylewarning and removed request for stylewarning January 30, 2017 22:53
Copy link
Contributor

@willzeng willzeng left a comment

Choose a reason for hiding this comment

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

Looks good @vontell Just a final few very minor changes and then LGTM!

##############################################################################

from pyquil.gates import X
from grove.qft.fourier import *
Copy link
Contributor

Choose a reason for hiding this comment

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

This * is saving you a little bit as you need to import more than just the X gate. I'd recommend the following change to make it clear where the imports are coming from:

from pyquil.gates import *
from grove.qft.fourier import inverse_qft
import pyquil.quil as pq

Copy link
Contributor

Choose a reason for hiding this comment

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

Its really nitpicky to list out all the gates, but you could do that too if you prefer

H(1), CPHASE(-1.5707963267948966, 1, 2),
H(2)])
print(trial_prog)
print(result_prog)
Copy link
Contributor

Choose a reason for hiding this comment

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

couple leftover print statements to remove

@@ -0,0 +1,50 @@
##############################################################################
#
# A collection of tests for the inverse quantum Fourier transform.
Copy link
Contributor

Choose a reason for hiding this comment

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

you can count this as a start of a test_qft.py so that we can add more tests to it rather than needing its own module for testing the inverse qft function

#
# A collection of tests for the inverse quantum Fourier transform.
#
# Written by Aaron Vontell on January 28th, 2017
Copy link
Contributor

Choose a reason for hiding this comment

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

Our policy is to just copy the standard header that is all of the other files. This means that we don't track authorship in source, but you will get to live in infamy here! https://github.com/rigetticomputing/grove/graphs/contributors

@willzeng willzeng merged commit a85f2e1 into rigetti:master Jan 30, 2017
@willzeng
Copy link
Contributor

Nice!

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.

None yet

3 participants