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

k-local Ising wrapper for QAOA #141

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

markf94
Copy link
Contributor

@markf94 markf94 commented Mar 30, 2018

This PR implements the following changes:

  • implements support for k-local Ising instances
  • fixes several bugs that occured with Ising instances that didn't have bias (h) terms for each qubit
  • introduces proper Read the Docs for the Ising QAOA wrapper
  • introduces a new parameter driver_operators for ising_qaoa() in order to allow for manual definition of the QAOA mixer hamiltonian
  • renames ising() to ising_qaoa()

markf94 and others added 20 commits March 30, 2018 00:35
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
Copy link
Contributor

@ampolloreno ampolloreno left a 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!

Here you can find documentation for the different functions of Ising QAOA.

grove.ising.ising_qaoa.ising_qaoa
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Contributor

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.

.. 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 \\).
Copy link
Contributor

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).
Copy link
Contributor

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?

Copy link
Contributor Author

@markf94 markf94 Jul 23, 2018

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.

Copy link
Contributor

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] \\)
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@ampolloreno ampolloreno left a 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.

: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.
Copy link
Contributor

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.

: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.
Copy link
Contributor

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.

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. "
Copy link
Contributor

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))?

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]])
Copy link
Contributor

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]

?

: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
Copy link
Contributor

Choose a reason for hiding this comment

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

:param list driver_operators:?

: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.
Copy link
Contributor

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?

@@ -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_]
Copy link
Contributor

Choose a reason for hiding this comment

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

No space before index.

driver_operators = []
# default to X mixer
for i in qubit_indices:
driver_operators.append(PauliSum([PauliTerm("X", i, -1.0)]))
Copy link
Contributor

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
@markf94
Copy link
Contributor Author

markf94 commented Jul 23, 2018

@ampolloreno Semaphore build fails due to unrelated issues with Tomography.

@ampolloreno
Copy link
Contributor

@markf94 Awesome! I'll get to looking at this again soon!

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

3 participants