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

quantum die program #749

Merged
merged 2 commits into from Jan 24, 2019
Merged

quantum die program #749

merged 2 commits into from Jan 24, 2019

Conversation

emilystamm
Copy link
Contributor

The current quantum die program in examples only rolls a die with the number of sides equal to a power of 2. So if for example the user inputs '5' for a 5-sided die, the result can be any integer from 1,...8. This program re-rolls the die if the result is greater than the number of sides.

Copy link
Contributor

@stylewarning stylewarning left a comment

Choose a reason for hiding this comment

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

Thank you so much for your contributions! I've written some comments. Many of them are stylistic, but important for consistency with the rest of the project. I highly recommend studying the source of the original file to see the style of it.

Lastly, how might you change the program so a user could run it on a real QPU as well?

@@ -1,79 +1,45 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not delete the header of this file, including the license and copyright notice.

Do note that your contributions are recorded as a part of the commit history.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also run pep8 on this file and make the necessary fixes to make it PEP8 conforming.

# ----------------------------
# Emily Stamm
# Dec 30, 2018

Copy link
Contributor

Choose a reason for hiding this comment

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

Please configure your editor to use 4 spaces, and not tabs, for indentation.

result += (2**i)*result_dict[i][0]
return result

# throw_polyhedral_die(num_sides)
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation strings should be written as a part of the Python docstring in """ triple quotes. Take a gander at other parts of the pyQuil source code, including the previous code in this file, to see how it works.


def throw_polyhedral_die(num_sides):
# Number of qubits needed for computation
num_qubits = int(np.ceil(np.log2(num_sides)))
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the qubits_needed function which you've deleted for this.

# Initialize program
p = Program()
p.inst(H(i) for i in range(num_qubits))

Copy link
Contributor

Choose a reason for hiding this comment

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

delete unnecessary newlines

qc_string = str(num_qubits) + 'q-qvm'
qc = get_qc(qc_string)

result = num_sides + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I would replace

result = ...
while f(result):
    result = ...
return result

with something more like

while True:
    if not f(result):
        return result

It's much simpler and doesn't require temporaries with broad scope.

import numpy as np
from pyquil.quil import *

def throw_polyhedral_die_helper(p,qc, num_qubits):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could choose a better name for this function. Also be sure to document it.

def throw_polyhedral_die_helper(p,qc, num_qubits):
result_dict = qc.run_and_measure(p, trials = 1)
result = 1
for i in range(num_qubits):
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be simpler to sum( ... ) over a comprehension here.

p.inst(H(i) for i in range(num_qubits))


qc_string = str(num_qubits) + 'q-qvm'
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if the logic of handling the (pseudo-)random numbers was separate from the logic of constructing the qvm, which was separate from the logic of rejection sampling. Just my 2 cents though.

print(f"The result is: {roll_die(qvm, number_of_sides)}")
print("The result is: ", throw_polyhedral_die(number_of_sides))

input_for_die()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the __main__ convention here.

@emilystamm
Copy link
Contributor Author

emilystamm commented Jan 6, 2019 via email

@notmgsk
Copy link
Contributor

notmgsk commented Jan 7, 2019

Hi @estamm12!

The pull request process is something like this:

  1. Fork the repo you're interested in (i.e. pyquil).
  2. Make changes to the code.
  3. Make a pull request to the original repo. Someone will review the PR and suggest any changes.
  4. You make changes to your fork, and these new changes will show up in the original PR.

You're currently at step 4. Make the changes @tarballs-are-good suggested, commit and push them to your pyquil fork, and then we come back to review and merging.

The standard textbook is Quantum Computation and Information by Nielsen and Chuang.

Copy link
Contributor

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

Thanks for adapting your code into the existing example! This current diff is beautifully short

@mpharrigan mpharrigan merged commit f1fe6de into rigetti:master Jan 24, 2019
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

4 participants