Fix issue when using a wire subset with `BasisState` #61

thisac · 2021-05-26T16:06:29Z

Fixes an issue when calling BasisState when using a subset of the device wires.

The following now works:

dev = qml.device("cirq.simulator", wires=2)

@qml.qnode(dev)
def circuit():
    qml.BasisState([1], wires=1)
    return qml.expval(qml.PauliZ(0)), qml.expval(qml.PauliZ(1))

circuit()

Previously it raised
DeviceError: For BasisState, the state has to be specified for the correct number of qubits. Got a state for 1 qubits, expected 2.

and now it should output the correct [1, -1].

…ane-cirq into fix-using-wire-subset

codecov · 2021-05-26T16:48:20Z

Codecov Report

Merging #61 (ff90b90) into master (14f421a) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage   99.10%   99.14%   +0.03%     
==========================================
  Files           8        8              
  Lines         334      349      +15     
==========================================
+ Hits          331      346      +15     
  Misses          3        3

Impacted Files	Coverage Δ
pennylane_cirq/simulator_device.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 14f421a...ff90b90. Read the comment docs.

thisac · 2021-05-26T16:50:29Z

pennylane_cirq/simulator_device.py

+        try:
+            wires = self.map_wires(qubit_state_vector_operation.wires)
+        except WireError:
+            wires = qubit_state_vector_operation.wires


I'm not sure why this is, but there are cases when QubitStateVector is called on more wires than the device has. E.g., where it's called on wires [0, 1] but the device only has 1 wire, as in the following test.

def test_var_single_wire_no_parameters( self, simulator_device_1_wire, tol, operation, input, expected_output ): """Tests that variances are properly calculated for single-wire observables without parameters.""" op = operation(0, do_queue=False) simulator_device_1_wire.reset() simulator_device_1_wire.apply( [qml.QubitStateVector(np.array(input), wires=[0, 1])], rotations=op.diagonalizing_gates(), )

This causes issues with map_wires (thus the try-except) and also the _expand_state below (thus the if len(wires) < self.num_wires ... instead of if len(wires) != self.num_wires ..., as I first wrote).

@thisac I would consider this a bug in the tests :) I suggest:

removing the try-except here

updating the test to use a 2 wire device

josh146

I did a quick skim through, looks good so far @thisac! I haven't done a proper review yet, as I noticed there are no tests (yet) to ensure that this bug is fixed

josh146 · 2021-05-26T17:21:30Z

pennylane_cirq/simulator_device.py

+        try:
+            wires = self.map_wires(qubit_state_vector_operation.wires)
+        except WireError:
+            wires = qubit_state_vector_operation.wires


@thisac I would consider this a bug in the tests :) I suggest:

removing the try-except here

updating the test to use a 2 wire device

josh146

Thanks @thisac! Tests look very nice.

Only suggestion is to use standard NumPy rather than the device static methods.

josh146 · 2021-05-27T06:30:52Z

pennylane_cirq/simulator_device.py

        self._initial_state = np.zeros(2 ** len(self.qubits), dtype=np.complex64)
        basis_state_idx = np.sum(2 ** np.argwhere(np.flip(basis_state_array) == 1))


Oh, this is a permutation of converting from base2 to base10 that I hadn't seen before!

Interestingly, however, this seems to be a rare case where the native Python approach is still faster:

>>> bs = np.array([1, 1, 0, 1, 1, 0, 0, 1, 1]) >>> int("".join(str(i) for i in bs), 2) 435 >>> np.sum(2 ** np.argwhere(np.flip(bs) == 1)) 435 >>> %timeit np.sum(2 ** np.argwhere(np.flip(bs) == 1)) 41.4 µs ± 4.82 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each) >>> %timeit int("".join(str(i) for i in bs), 2) 14.4 µs ± 489 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

Interesting! I feel like the Python solution is somewhat cleaner. The time spent there is almost all in the "".join while the NumPy solution spends a significant amounts of time in np.argwhere, 2 ** and np.sum. I guess there's just more that's going on there. 🤔

josh146 · 2021-05-27T06:31:31Z

pennylane_cirq/simulator_device.py

+        state_vector = self._scatter(ravelled_indices, state_vector, [2 ** self.num_wires])
+        state_vector = self._reshape(state_vector, [2] * self.num_wires)
+        state_vector = self._asarray(state_vector, dtype=self.C_DTYPE)
+


Same comment here as over on the Qulacs PR, recommend changing this to simply use native NumPy functions :)

josh146 · 2021-05-27T06:32:00Z

tests/test_simulator_device.py

+    dev = SimulatorDevice(device_wires)
+    res = dev._expand_state(state, op_wires)
+
+    assert np.allclose(res, expected, **tol)


great tests!

thisac added 2 commits May 26, 2021 11:40

add expand_state

632c71c

move basis state checks

845ff57

thisac requested review from josh146 and antalszava May 26, 2021 16:06

thisac self-assigned this May 26, 2021

thisac added 4 commits May 26, 2021 12:08

update changelog

e19b129

Merge branch 'master' into fix-using-wire-subset

1a7b49c

update apply state vector

17c7e75

Merge branch 'fix-using-wire-subset' of github.com:PennyLaneAI/pennyl…

53d18e8

…ane-cirq into fix-using-wire-subset

thisac commented May 26, 2021

View reviewed changes

josh146 reviewed May 26, 2021

View reviewed changes

thisac added 5 commits May 26, 2021 15:11

add tests

4fb594d

add expand state test

ff83b4d

remove try-except

d5f2826

fix single-qubit var tests

52269b6

remove import

dc88464

thisac requested a review from josh146 May 26, 2021 20:03

josh146 approved these changes May 27, 2021

View reviewed changes

apply changes from code review

ff90b90

thisac merged commit 6fe3a57 into master May 27, 2021

thisac deleted the fix-using-wire-subset branch May 27, 2021 17:54

antalszava mentioned this pull request Jul 6, 2021

Incorrect state preparations for multiple qubits #60

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue when using a wire subset with `BasisState` #61

Fix issue when using a wire subset with `BasisState` #61

thisac commented May 26, 2021

codecov bot commented May 26, 2021 •

edited

Loading

thisac May 26, 2021 •

edited

Loading

josh146 May 26, 2021

josh146 left a comment

josh146 May 26, 2021

josh146 left a comment

josh146 May 27, 2021

thisac May 27, 2021

josh146 May 27, 2021

josh146 May 27, 2021

		self._initial_state = np.zeros(2 ** len(self.qubits), dtype=np.complex64)
		basis_state_idx = np.sum(2 ** np.argwhere(np.flip(basis_state_array) == 1))

Fix issue when using a wire subset with BasisState #61

Fix issue when using a wire subset with BasisState #61

Conversation

thisac commented May 26, 2021

codecov bot commented May 26, 2021 • edited Loading

Codecov Report

thisac May 26, 2021 • edited Loading

Choose a reason for hiding this comment

josh146 May 26, 2021

Choose a reason for hiding this comment

josh146 left a comment

Choose a reason for hiding this comment

josh146 May 26, 2021

Choose a reason for hiding this comment

josh146 left a comment

Choose a reason for hiding this comment

josh146 May 27, 2021

Choose a reason for hiding this comment

thisac May 27, 2021

Choose a reason for hiding this comment

josh146 May 27, 2021

Choose a reason for hiding this comment

josh146 May 27, 2021

Choose a reason for hiding this comment

Fix issue when using a wire subset with `BasisState` #61

Fix issue when using a wire subset with `BasisState` #61

codecov bot commented May 26, 2021 •

edited

Loading

thisac May 26, 2021 •

edited

Loading