[QAOA] Adds Built-In Mixer Hamiltonians #712
Conversation
Codecov Report
@@ Coverage Diff @@
## master #712 +/- ##
==========================================
+ Coverage 95.46% 95.47% +0.01%
==========================================
Files 107 109 +2
Lines 6791 6813 +22
==========================================
+ Hits 6483 6505 +22
Misses 308 308
Continue to review full report at Codecov.
|
Lucaman99
changed the title
Lucaman99
changed the title
There was a problem hiding this comment.
Thanks @Lucaman99, I've given it a quick look. My main question is about the motivation for the Iterable type for the input graph?
pennylane/qaoa/qaoa.py
Outdated
|
|
||
|
|
||
| def check_iterable_graph(graph): | ||
| """ Checks if a graph supplied in 'Iterable format' is formatted correctly |
There was a problem hiding this comment.
What does the iterable format look like? Could be worth adding a note for the developer, even if this function becomes non-user-facing
pennylane/qaoa/qaoa.py
Outdated
| """ Checks if a graph supplied in 'Iterable format' is formatted correctly | ||
|
|
||
| Args: | ||
| graph (Iterable): The graph that is being checked |
There was a problem hiding this comment.
Worth adding "Raises: ..." since the function doesn't return anything?
pennylane/qaoa/qaoa.py
Outdated
| for some graph :math:`G`. :math:`X_i` and :math:`Y_i` denote the Pauli-X and Pauli-Y operators on the :math:`i`-th | ||
| qubit respectively. | ||
| Args: | ||
| graph (Iterable or networkx.Graph) A graph defining the pairs of qubits (wires) on which each term of the Hamiltonian acts. |
There was a problem hiding this comment.
Is there a motivation for supporting the Iterable type? I'd be tempted to just support NetworkX Graph type, which is pretty established.
There was a problem hiding this comment.
This choice was based off-of Nathan's comments on the QAOA ADR, where he said that supporting both different input types for graphs would be nice.
There was a problem hiding this comment.
What is the iterable type? Simply a list of edges of the form g = [(0, 1), (2, 6), ...]?
There was a problem hiding this comment.
@josh146 It can theoretically be any Iterable with Iterable pairs as elements
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
There was a problem hiding this comment.
Thanks @Lucaman99, approved provided the two comments are considered:
- Adding to
pennylane.__init__ - Alphabetical order in the docs toc
💯
|
Hey @trbromley @josh146 @ixfoduap - I just made a small, yet pretty significant change to the code. Basically, we have decided that having Hamiltonians track the user's physical wires within a QAOA workflow is the best course of action. Thus, the mixer Hamiltonians now take a list of wires as an argument, rather than an integer representing the number of wires on which the Hamiltonian acts. Other than this, nothing else has changed! |
There was a problem hiding this comment.
Thanks @Lucaman99! I just have two questions:
- What about a test for non-consecutive wires case?
- For the
xy_mixer()function, is the idea that the input graph replaces theWires? I think that makes sense, although I was also expecting to seewiresas an input in that function 🤔
| class TestMixerHamiltonians: | ||
| """Tests that the mixer Hamiltonians are being generated correctly""" | ||
|
|
||
| def test_x_mixer_output(self): |
There was a problem hiding this comment.
How about adding a parametrize over this function that tries two cases, one with consecutive and one with non-consecutive wires?
|
@trbromley Thanks! I'll definitely add a test for non-consecutive wires! As for the |
|
@trbromley I just added the non-consecutive wires test! |
|
@Lucaman99, did you mean to do the force push? |
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Context:
These changes are a part of the new QAOA functionality that is being introduced into PennyLane.
Description of the Change:
test_qaoa.pyBenefits:
Possible Drawbacks:
Related GitHub Issues: