-
Notifications
You must be signed in to change notification settings - Fork 124
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
k-local Ising wrapper for QAOA #141
base: master
Are you sure you want to change the base?
Conversation
Ising instances can have many variables but almost no bias terms so a dictionary is naturally a better choice in order to account for sparse bias vectors
Ising instances can make use of arbitrary qubits especially when we have to map to a QPU hardware graph. Makes much more sense to extract the qubit indices and compute n_nodes like this.
allows for user-defined mixer/driver hamiltonian in order to enforce hard constraints tests & docs also updated
added extensive documentation for the Ising QAOA wrapper added images added ising_qaoa to index.rst
added extensive documentation for the Ising QAOA wrapper added images added ising_qaoa to index.rst
3a82ff7
to
af3cd11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markf94, thank for the PR - and sorry for the (extreme) delay in reviewing. I'd really like to get this merged in.
I've commented in a couple places in the docs for starters, one particularly troubling thing to me is that the examples in the docs seem to return different results than those listed when I run them.
If you don't mind, I'll try to push changes as I go (time permitting), so we can get this merged in.
Thanks again, and looking forward to getting this merged int!
docs/ising_qaoa.rst
Outdated
Here you can find documentation for the different functions of Ising QAOA. | ||
|
||
grove.ising.ising_qaoa.ising_qaoa | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sphinx is complaining here,
/home/ampolloreno/repos/grove/docs/ising_qaoa.rst:256: WARNING: Title underline too short.
Extend the tildes, please.
docs/ising_qaoa.rst
Outdated
.. image:: ising_qaoa/triangle_desired.png | ||
:align: center | ||
:scale: 75% | ||
Let's again define that we colour vertex \\( i \\) black if \\( \\sigma_{i} = -1 \\) and white if \\( \\sigma_{i} = +1 \\). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sphinx complained here too.
I think it wants a newline after the image block.
/home/ampolloreno/repos/grove/docs/ising_qaoa.rst:218: WARNING: Explicit markup ends without a blank line; unexpected unindent.
stats[tuple(solution_string)] = 1 | ||
print(f'Solution statistics: {stats}') | ||
|
||
You should see that the algorithm returns the aforementioned solution with ~99% probability (unless the classical minimizer got stuck in a local minima). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this when I run the routine - has something changed that breaks this example since you made this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ampolloreno the problem was the declaration of driver_operators
as an empty list before checking if it is None
which resulted in no driver Hamiltonian. This has been resolved in commit fc381d4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I'll check it out!
stats[tuple(solution_string)] = 1 | ||
print(f'Solution statistics: {stats}') | ||
|
||
You should get the two possible solution strings \\( [-1, 1, 1, -1] \\) and \\( [1, -1, -1, 1] \\) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ampolloreno This has also been resolved in commit fc381d4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markf94 Some more comments.
grove/ising/ising_qaoa.py
Outdated
:param h: External magnectic term of the Ising problem. List. | ||
:param J: Interaction term of the Ising problem. Dictionary. | ||
:param sol: Ising solution. List. | ||
:param h: (dict) External magnetic term of the Ising problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically the type goes inside of the parameter specifier, e.g. :param dict h:
, I don't know if sphinx renders the type the way it's written here.
grove/ising/ising_qaoa.py
Outdated
:param sol: Ising solution. List. | ||
:param h: (dict) External magnetic term of the Ising problem. | ||
:param J: (dict) Interaction terms of the Ising problem (may be k-local). | ||
:param sol: (list) Ising solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should say as returned from the QPU/QVM, given that the code depends on that ordering, and reverses it.
grove/ising/ising_qaoa.py
Outdated
raise TypeError("""Interaction term must connect two different variables""") | ||
paired_indices = [(a, b) for a, b in zip(elm, elm)] | ||
if len(paired_indices) != len(set(paired_indices)): | ||
raise TypeError("Interaction term must connect different variables. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To catch this can't we just check len(elm) == len(set(elm))
?
grove/ising/ising_qaoa.py
Outdated
else: | ||
ener_ising += J[elm] * int(sol[elm[0]]) * int(sol[elm[1]]) | ||
for i in range(len(h)): | ||
multipliers = int(sol[elm[0]]) * int(sol[elm[1]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what's special about the first two elements, can we instead do:
multipliers = 1
for idx in elm:
multipliers *= sol[idx]
?
grove/ising/ising_qaoa.py
Outdated
:param num_steps: (Optional.Default=2 * len(h)) Trotterization order for the | ||
QAOA algorithm. | ||
:param driver_operators: (Optional. Default: X on all qubits.) The mixer/driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:param list driver_operators:
?
grove/ising/ising_qaoa.py
Outdated
:param verbose: (Optional.Default=True) Verbosity of the code. | ||
:param rand_seed: (Optional. Default=None) random seed when beta and | ||
gamma angles are not provided. | ||
:param connection: (Optional) connection to the QVM. Default is None. | ||
:param samples: (Optional. Default=None) VQE option. Number of samples | ||
(circuit preparation and measurement) to use in operator | ||
averaging. | ||
averaging. Required when using QPU backend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only required for the QPU? Don't the QVM and QPU both have default num_trials
?
grove/ising/ising_qaoa.py
Outdated
@@ -82,18 +99,31 @@ def ising(h, J, num_steps=0, verbose=True, rand_seed=None, connection=None, samp | |||
if num_steps == 0: | |||
num_steps = 2 * len(h) | |||
|
|||
n_nodes = len(h) | |||
qubit_indices = set([ index for tuple_ in list(J.keys()) for index in tuple_] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No space before index
.
grove/ising/ising_qaoa.py
Outdated
driver_operators = [] | ||
# default to X mixer | ||
for i in qubit_indices: | ||
driver_operators.append(PauliSum([PauliTerm("X", i, -1.0)])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a -1?
- fixed doc strings - fixed spacing according to PEP8
- declaring driver_operators as empty list led to no driver Hamiltonian being used
@ampolloreno Semaphore build fails due to unrelated issues with Tomography. |
@markf94 Awesome! I'll get to looking at this again soon! |
This PR implements the following changes:
driver_operators
for ising_qaoa() in order to allow for manual definition of the QAOA mixer hamiltonian