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

[QAOA] Adds Cost and Mixer Layers #720

Merged
merged 39 commits into from
Aug 7, 2020
Merged

Conversation

Lucaman99
Copy link
Contributor

@Lucaman99 Lucaman99 commented Jul 24, 2020

Context:

Part of the ongoing efforts to add QAOA functionality to PennyLane

Description of the Change:

Adds methods for constructing QAOA cost and mixer layers, along with tests.

Note: This functionality depends on the features introduced in a PR that is currently open (#710). I added some of this functionality to this PR (so that tests will pass). However, this PR should definitely not be merged until #710 is, at which point I will remove the extraneous functionality from this PR.

Related

#718, #712, #710

@codecov
Copy link

codecov bot commented Jul 24, 2020

Codecov Report

Merging #720 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #720      +/-   ##
==========================================
+ Coverage   95.62%   95.63%   +0.01%     
==========================================
  Files         111      112       +1     
  Lines        6945     6968      +23     
==========================================
+ Hits         6641     6664      +23     
  Misses        304      304              
Impacted Files Coverage Δ
...ane/templates/subroutines/approx_time_evolution.py 100.00% <ø> (ø)
pennylane/qaoa/__init__.py 100.00% <100.00%> (ø)
pennylane/qaoa/layers.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d584f9...09a1efb. Read the comment docs.

@josh146
Copy link
Member

josh146 commented Jul 26, 2020

@Lucaman99, if it makes more sense, we can change the 'base branch' for this PR from master to the branch used for #710. This will avoid the need to add the extraneous code.

@Lucaman99
Copy link
Contributor Author

@josh146 Awesome, I didn't realize that could be done!

@josh146
Copy link
Member

josh146 commented Jul 27, 2020

No worries! Just click on the 'edit' button on the top:

image

and then you can change the base branch 👍
image

@Lucaman99
Copy link
Contributor Author

@josh146 I tried to do this, and I don't think it's possible, since the branch in #710 is part of my personal fork.

@josh146
Copy link
Member

josh146 commented Jul 31, 2020

@josh146 I tried to do this, and I don't think it's possible, since the branch in #710 is part of my personal fork.

Oh :(

@Lucaman99
Copy link
Contributor Author

For the time being, I had to take the ApproxTimeEvolution file and add it to the PR (in order for the tests to pass). Since I didn't bother adding the corresponding tests, Codecov is failing.

@Lucaman99 Lucaman99 marked this pull request as ready for review August 5, 2020 13:47
return val


def cost_layer(hamiltonian):
Copy link
Contributor

@ixfoduap ixfoduap Aug 5, 2020

Choose a reason for hiding this comment

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

I find it a bit strange at first sight that the layer does not take gamma as an input. I understand that we call ApproxTimeEvolution as a lambda function, but I'm wondering if this is the best choice.

Alternatively we could use def cost_layer(hamiltonian, gamma): and then use

from pennylane.qaoa import cost_layer
@qml.qnode(dev)
def circuit(gamma):
    cost_layer(cost_h, gamma)

I'm not sure which approach is best. Curious what others think?

Copy link
Member

Choose a reason for hiding this comment

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

I am ambivalent 🤔

I see pros and cons for both approaches:

  • In the cost_layer(cost_h, gamma) approach, cost_layer is essentially a template, and must always be called within a QNode.

  • In the cost = cost_layer(cosh_h) approach, cost_layer is more like a 'template generating function'. The returned cost function can only be used within a QNode, like a template.

Copy link
Member

Choose a reason for hiding this comment

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

Probably lean more towards cost_layer(cost_h, gamma), unless you see a use-case for having the 'template generating' behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ixfoduap @josh146 Sounds good, I'll make the change!

Copy link
Contributor

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

My main suggestions are to include the variational parameter as input and to add tests that are independent from ApproxTimeEvolution

pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
tests/test_qaoa.py Outdated Show resolved Hide resolved
"""Tests that the gates of the mixer layer is correct"""

alpha = 1
hamiltonian = qml.Hamiltonian([1, 1], [qml.PauliX(0), qml.PauliX(1)])
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand, this is testing that the layers are implementing the same sequence of gates as ApproxTimeEvolution, which I'm not sure is the best way to test since ApproxTimeEvolution may be giving the wrong answers, i.e., these tests don't add much more than the tests for ApproxTimeEvolution. Not sure if this is ok...

Maybe it's good to test an output circuit explicitly, without relying on ApproxTimeEvolution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made these changes 🙂

Lucaman99 and others added 2 commits August 5, 2020 17:09
Co-authored-by: ixfoduap <40441298+ixfoduap@users.noreply.github.com>
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Nice work @Lucaman99 🔥

pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
return val


def cost_layer(hamiltonian):
Copy link
Member

Choose a reason for hiding this comment

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

I am ambivalent 🤔

I see pros and cons for both approaches:

  • In the cost_layer(cost_h, gamma) approach, cost_layer is essentially a template, and must always be called within a QNode.

  • In the cost = cost_layer(cosh_h) approach, cost_layer is more like a 'template generating function'. The returned cost function can only be used within a QNode, like a template.

return val


def cost_layer(hamiltonian):
Copy link
Member

Choose a reason for hiding this comment

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

Probably lean more towards cost_layer(cost_h, gamma), unless you see a use-case for having the 'template generating' behaviour.

pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
pennylane/qaoa/layers.py Show resolved Hide resolved
pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
Co-authored-by: Josh Izaac <josh146@gmail.com>
@co9olguy co9olguy added the do not merge ⚠️ Do not merge the pull request until this label is removed label Aug 6, 2020
Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @Lucaman99, just left a few comments but looks good!

pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
pennylane/qaoa/layers.py Outdated Show resolved Hide resolved
pennylane/qaoa/layers.py Show resolved Hide resolved
return val


def cost_layer(gamma, hamiltonian):
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it is already an abuse of style, but the other templates use the "CapitalizedWords" class-like case style (even though they are also functions). Is there a motivation for breaking this tradition here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point! @ixfoduap @josh146 what do you think?

Copy link
Contributor Author

@Lucaman99 Lucaman99 Aug 7, 2020

Choose a reason for hiding this comment

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

@trbromley I talked to Josh, and he is Ok with either. @ixfoduap do you have a preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the suggestion Tom! I view this more as a function inside qaoa than a template, so I prefer not using capitalized naming here

],
[
qaoa.xy_mixer(Graph([(0, 1), (1, 2), (2, 0)])),
[qml.PauliRot(1, "XX", wires=[0, 1]), qml.PauliRot(1, "YY", wires=[0, 1]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I never saw this class before! Maybe we need to add it to https://pennylane.readthedocs.io/en/latest/introduction/operations.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely! MultiRZ should probably be added as well

Copy link
Member

Choose a reason for hiding this comment

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

@trbromley @Lucaman99 Let's make sure that the documentation is fully up to date next week during the feature freeze :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josh146 Sounds good!

Lucaman99 and others added 5 commits August 6, 2020 15:31
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
return val


def cost_layer(gamma, hamiltonian):
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the suggestion Tom! I view this more as a function inside qaoa than a template, so I prefer not using capitalized naming here

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Great job!

@ixfoduap ixfoduap removed the do not merge ⚠️ Do not merge the pull request until this label is removed label Aug 7, 2020
@ixfoduap ixfoduap merged commit 27b1bf6 into PennyLaneAI:master Aug 7, 2020
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

5 participants