Skip to content

Conversation

Lucaman99
Copy link
Contributor

@Lucaman99 Lucaman99 commented Aug 12, 2019

I created a Jupyter Notebook tutorial on simulating quantum random walks using Cirq, and added it to the examples folder of the repository. The reason there are so many commits to my branch is because the Notebook formatting on Github was a lot different than when I ran the file locally, and thus I needed to do a lot of editing. 😄

Fixes #1564

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@vtomole
Copy link
Collaborator

vtomole commented Aug 12, 2019

@vtomole
Copy link
Collaborator

vtomole commented Aug 12, 2019

I'm looking into https://pypi.org/project/pytest-ipynb/ to see if it would be a better way of testing than the Openfermion-Cirq example.

Copy link
Collaborator

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

Remove all instances of \biggr as it causes the vertical line to be too long. Also, break up your writing into more lines so they don't go off the page when you edit

For example this:
off

Should be this
on

@Strilanc Strilanc changed the title Added Quantum Random Walk Tutorial Notebook Add Quantum Random Walk Tutorial Notebook Aug 22, 2019
@Lucaman99
Copy link
Contributor Author

Lucaman99 commented Sep 12, 2019

Hi @vtomole - I made some of the changes you requested! Sorry, I'm fairly new at using the Github collaboration features, I made the changes that you requested manually, but now I'm realizing that you may have already made the changes and I never accepted them.

Also, I'm a little bit unclear as to what the Jupyter Notebook tests do and how they should be added into my pull request, I would really appreciate any clarification!

Thank you!

@vtomole
Copy link
Collaborator

vtomole commented Sep 25, 2019

@Lucaman99 You haven't signed the CLA yet.

@Lucaman99
Copy link
Contributor Author

When I go to the link it says I have signed it

@vtomole
Copy link
Collaborator

vtomole commented Sep 25, 2019

@Lucaman99
Okay then make a comment "@googlebot I fixed it.." If the bot doesn't comment, it means it doesn't think anything has changed.

@Lucaman99
Copy link
Contributor Author

@googlebot I fixed it.

@vtomole
Copy link
Collaborator

vtomole commented Sep 25, 2019

@Lucaman99 Is the email in your commits different from the one you used to sign the CLA?

@Lucaman99
Copy link
Contributor Author

@vtomole Yeah, I think so, the CLA won't allow me to change the email

@vtomole
Copy link
Collaborator

vtomole commented Sep 25, 2019

@Lucaman99 I've committed from different emails before with no problems. @Strilanc any tips?

@Lucaman99
Copy link
Contributor Author

@vtomole Should I just create another branch and submit a pull request with the correct email?

@vtomole
Copy link
Collaborator

vtomole commented Sep 25, 2019

@Lucaman99 Did adding the other email to your Github account not work?

@Lucaman99
Copy link
Contributor Author

No, I don't think so

@Lucaman99
Copy link
Contributor Author

@googlebot I fixed it.

@vtomole
Copy link
Collaborator

vtomole commented Sep 25, 2019

@Lucaman99 No, we can override the cla. You can test if the email account problem is correct by making a "fake" pull request with the "correct" email commit and we'll see if it accepts.

Copy link
Contributor

@Strilanc Strilanc left a comment

Choose a reason for hiding this comment

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

Sorry for not reviewing this earlier. I think that this is a good start, but I do have several comments.

There's spurious $$ signs in the equation before the quantum random walks section.

A quantum random walk should just be called a quantum walk, I think. @dabacon do you know the standard terminology? In the past I've also been tempted to call it a quantum random walk, but it's really not "random". The superposition updates in a deterministic way.

An overarching criticism that I have of the current way the notebook is laid out is that it should be providing executable python code much earlier. A notebook is a teaching mechanism that leverages executable code, so it's odd to tack the code on at the end instead of weaving it throughout. When you're explaining what a classical random walk is, what the distribution is, you could be generating that distribution from actual random walks.

First code snippet: use cirq.GridQubit.rect.

Second code snippet: how about just cirq.X(target).controlled_by(*controls) instead of the method decomposing it into Toffolis?

For the controlled increment/decrement operation, you could take advantage of cirq's new ArithmeticOperation class. You could then start digging down more into the decomposition if necessary.

A lot of the code snippets can be simplified. For example, x_arr = [j for j in dict(final).keys()] is just x_arr = list(final.keys()).

@@ -0,0 +1,766 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it shouldn't have been included in the commit. Remove it. You can add an */.ipynb_checkpoints/* entry to the .gitignore file to ensure it doesn't get recommitted.

@Lucaman99
Copy link
Contributor Author

@vtomole Yes, I just tried it, and it still didn't seem to work (It said it found a CLA for the author of the pull request, which is my account with the correct email, but can't find one for all the contributors)

@Strilanc
Copy link
Contributor

Try squashing everything down into one commit

git checkout master -b fresh_branch
git merge old_branch --squash
git commit -m "everything in one commit"

You may need to configure your email into git. Check git config user.email and git config --global user.email.

@Lucaman99
Copy link
Contributor Author

Hey @Strilanc - I read over your review, and I'm so sorry for the massive delay in fixing these issues. I have the bandwidth to fix this hopefully over the next week or so. All of the minor issues will definitely be fixed ASAP. In regards to your overarching criticism, it is totally valid, and I agree. I'll change you structure of the notebook with some of your suggestions. For the multi-controlled-X gates and the arithmetic operations, I'll change them to the new methods, however, I think this is a good opportunity to provide intuition about why these things actually work, and ultimately ensure the person reading this tutorial is confident the quantum walk can be implemented from scratch, with the base set of gates. I'll probably keep the methods I wrote separate from the main quantum walk simulation, and talk a little bit about the decompositions after I introduce the built-in functions. Would that be Ok with you?

@vtomole
Copy link
Collaborator

vtomole commented Dec 11, 2019

@Lucaman99 That's fine to me.

@Lucaman99
Copy link
Contributor Author

I just finished the latest version, will push tonight.

@Lucaman99
Copy link
Contributor Author

Added a PR: #2657 😃

@vtomole
Copy link
Collaborator

vtomole commented Feb 18, 2020

Closing as there is another PR that attempts to accomplish this.

@vtomole vtomole closed this Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create an example using a tested jupyter notebook
4 participants