added tensor_up feature by ncrubin · Pull Request #2 · rigetti/reference-qvm

ncrubin · 2017-10-05T16:58:13Z

the tensor_up() method allows the user to easily construct the matrix
form of operators represented as PauliSums. Though there is a slight
degree of overlap with some of the existing functionality in
unitary_generators, this method provides a very easy interface to the
users of pyQuil and reference-qvm to examine operators and Hamiltonains.

In the future when the gate matrices are converted to sparse operators
we can use sparse kron in scipy to speed up construction of these
operators. Right now there is a lot of wasted computation by
multiplying by known zeros.

the `tensor_up()` method allows the user to easily construct the matrix form of operators represented as PauliSums. Though there is a slight degree of overlap with some of the existing functionality in unitary_generators, this method provides a very easy interface to the users of pyQuil and reference-qvm to examine operators and Hamiltonains. In the future when the gate matrices are converted to sparse operators we can use sparse kron in scipy to speed up construction of these operators. Right now there is a lot of wasted computation by multiplying by known zeros.

jotterbach

Some minor things about coding style and suggestions for testing methods. Otherwise LGTM!

Should we introduce a sanity check to not kill the program if we tensor_up too many terms?

jotterbach · 2017-10-05T17:15:40Z

+    if not isinstance(pauli_terms, PauliSum):
+        raise TypeError("can only tensor PauliSum")
+
+    if __debug__:


do we need to check in debug code?

under default mode of running python (without -O), then this check occurs. If run with -O the code is trusting the user to not send any PauliSums with non-identity terms on qubits with higher than the maximum. Do you think the check should run for all mode's of operation? or just when the user expects safe and sane execution.

I think this check should always occur. It's cheap as the kroning operation is not executed in a loop over-and-over again if the circuit code is produced in a reasonable way.

jotterbach · 2017-10-05T17:20:14Z

+        for index in range(num_qubits):
+            pauli_mat = gate_matrix[term[index]]
+
+            tmp_big_hilbert = np.kron(pauli_mat, tmp_big_hilbert)


Collapse this in one line. The additional assignment does not increase readability.

tmp_big_hilbert = np.kron(gate_matrix[term[index]], tmp_big_hilbert)

jotterbach · 2017-10-05T17:22:33Z

+    big_hilbert = np.zeros((2 ** num_qubits, 2 ** num_qubits))
+    # left kronecker product corresponds to the correct basis ordering
+    for term in pauli_terms.terms:
+        tmp_big_hilbert = np.array([1])


move this in front of the for loop to visually indicate that they belong together and form a fold operation.
moreover this assignment is too heavy. A simple
tmp_big_hilbert = 1 will do the trick

The np.kron routine uses np.asarray on both inputs to cast as an array object. Just being explicit about the type of object that I am performing the tensor product with--i.e. both objects are numpy.ndarray.

jotterbach · 2017-10-05T17:24:08Z

+
+        tmp_big_hilbert = tmp_big_hilbert * term.coefficient
+
+        big_hilbert = big_hilbert + tmp_big_hilbert


lots of nested logic in these two assignments. Try

big_hilbert += tmp_big_hilbert * term.coefficient

which gets rid of a lot of lines and is (arguably) more readable

👍 for in-place update of big_hilbert. wasted allocation steps before. Requires initialization of big_hilbert to have the appropriate type (complex). good call.

jotterbach · 2017-10-05T17:24:58Z

+    qubits and returns a matrix corresponding the tensor representation of the
+    object.
+
+    Useful for generating the full Hamiltonian after a particular fermion to


Doesn't clarify the use case. If you say that then you should give an example

jotterbach · 2017-10-05T17:27:28Z

+    # test correctness
+    trial_matrix = tensor_up(xy_term, 2)
+    true_matrix = np.kron(gate_matrix['Y'], gate_matrix['X'])
+    assert np.allclose(trial_matrix, true_matrix)


don't use np.allclose with asserts. There is better functionality in numpy. Try

np.testing.assert_allclose(trial_matrix, true_matrix)

ass this not only raises assertion error but also tells you more details of what is wrong.

jotterbach · 2017-10-05T17:27:33Z

+    x1_term = PauliSum([PauliTerm("X", 1)])
+    trial_matrix = tensor_up(x1_term, 2)
+    true_matrix = np.kron(gate_matrix['X'], gate_matrix['I'])
+    assert np.allclose(trial_matrix, true_matrix)


jotterbach · 2017-10-05T17:27:39Z

+    true_matrix = np.zeros((4, 4))
+    true_matrix[0, 0] = 2
+    true_matrix[-1, -1] = -2
+    assert np.allclose(trial_matrix, true_matrix)


jotterbach · 2017-10-05T17:28:24Z

    assert np.allclose(test_unitary, true_unitary)
+
+
+def test_tensor_up():


this test has a lot of cases. You should make this a bit more atomic. Maybe split it up in the error testing cases and the correctness cases

jotterbach · 2017-10-05T17:30:13Z

+    """
+    if not isinstance(pauli_terms, PauliSum):
+        raise TypeError("can only tensor PauliSum")
+


You might wanna check for sanity if the operator gets too big. If you kron up 100 terms then the final matrix will be gargantuan and might kill you.
Otherwise you can always check for sparsity of the gate_matrices.

We could do that. I believe in the qam.py there was a hard coded limit of 51 qubits (way beyond what could be represented with a numpy array on a typical machine). I advocated against putting a limit because the formal structure of the QAM doesn't have a limit.

I can see that argument. Maybe we can find a middle ground and generate a warning (https://docs.python.org/3/library/warnings.html#module-warnings) beyond some semi-reasonable limit. This will not interrupt the code execution but warn the user that code with large number of qubits might have a performance issue

1. Example in docstring 2. Separated testing and use of numpy testing infrastructure 3. cleaned up kronecker product and explicit array types.

jotterbach

A few comments, but not blocking. Lemme know what you think

jotterbach · 2017-10-05T23:15:44Z

+    """
+    if not isinstance(pauli_terms, PauliSum):
+        raise TypeError("can only tensor PauliSum")
+


I can see that argument. Maybe we can find a middle ground and generate a warning (https://docs.python.org/3/library/warnings.html#module-warnings) beyond some semi-reasonable limit. This will not interrupt the code execution but warn the user that code with large number of qubits might have a performance issue

jotterbach · 2017-10-05T23:17:29Z

+    if not isinstance(pauli_terms, PauliSum):
+        raise TypeError("can only tensor PauliSum")
+
+    if __debug__:


I think this check should always occur. It's cheap as the kroning operation is not executed in a loop over-and-over again if the circuit code is produced in a reasonable way.

jotterbach · 2017-10-07T01:33:33Z

Looks good to me. I'm shipping it!

ncrubin requested a review from jotterbach October 5, 2017 17:05

jotterbach suggested changes Oct 5, 2017

View reviewed changes

jotterbach assigned ncrubin Oct 5, 2017

PR review changes

569c750

1. Example in docstring 2. Separated testing and use of numpy testing infrastructure 3. cleaned up kronecker product and explicit array types.

jotterbach reviewed Oct 5, 2017

View reviewed changes

PR final changes

f934ca6

jotterbach approved these changes Oct 7, 2017

View reviewed changes

jotterbach merged commit d034763 into rigetti:master Oct 7, 2017


		tmp_big_hilbert = tmp_big_hilbert * term.coefficient

		big_hilbert = big_hilbert + tmp_big_hilbert

		assert np.allclose(test_unitary, true_unitary)


		def test_tensor_up():

Conversation

ncrubin commented Oct 5, 2017

Uh oh!

jotterbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncrubin Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncrubin Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jotterbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jotterbach commented Oct 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ncrubin Oct 5, 2017 •

edited

Loading

ncrubin Oct 5, 2017 •

edited

Loading