Fix cirq.Simulator assuming a reset is safe to cache before sampling #3093

Strilanc · 2020-06-16T00:03:26Z

This resulted in some circuits with random behavior giving long lists of identical samples when repeated
Change the general strategy used by the simulator to one of extracting a unitary prefix that can be cached, and then looking at the rest to decide whether sampling can be done efficiently or not

- This resulted in some circuits with random behavior giving long lists of identical samples when repeated - Change the general strategy used by the simulator to one of extracting a unitary prefix that can be cached, and then looking at the rest to decide whether sampling can be done efficiently or not

…andom # Conflicts: # cirq/sim/sparse_simulator.py

dabacon

LGTM only one minor comment.

dabacon · 2020-06-17T20:23:53Z

cirq/sim/sparse_simulator.py

+        general_ops = list(general_suffix.all_operations())
+        if all(isinstance(op.gate, ops.MeasurementGate) for op in general_ops):
+            return step_result.sample_measurement_ops(measurement_ops=cast(
+                List[ops.GateOperation], general_ops),


autoformat is on drugs

We cannot question it, we can only follow.

dabacon · 2020-06-17T20:26:53Z

cirq/sim/sparse_simulator.py

@@ -331,3 +336,27 @@ def sample(self,
                                                    self, None),
                                                repetitions=repetitions,
                                                seed=seed)
+
+
+def _split_into_unitary_then_general(circuit: 'cirq.Circuit'


There is probably a case to be made for a good way to handle this sort of splitting of circuits by disjoint filter as part of Cirq. No need to do now but maybe we should have an issue for this?

Acknowledged.

dabacon · 2020-06-17T20:41:20Z

cirq/sim/sparse_simulator.py

-                initial_state=0,
-                perform_measurements=False):
+        qubit_order = sorted(resolved_circuit.all_qubits())
+


just to make sure I understand: the bug was that intermediate channel ops that could be monte carlo sampled were allowed to not be terminal, and the new way this works it splits into the biggest possible unitary chunk and if then "others" to figure out whether this is possible.

Yes, that was the issue.

dstrain115 · 2020-06-17T03:02:09Z

cirq/sim/simulator.py

+        seen_qubits: Set[cirq.Qid] = set()
+        for op in measurement_ops:
+            for q in op.qubits:
+                if q not in seen_qubits:


Can't you just do something like this?

for op in measurement_ops: for q in op.qubits: seen_qubits.add(q) measured_qubits = list(seen_qubits)

This is what I initially did, and it resulted in several fixed-seed tests being flaky. The problem is it results in a non-deterministic ordering because set doesn't guarantee insertion order ordering.

dstrain115 · 2020-06-17T20:41:47Z

cirq/sim/sparse_simulator.py

+
+    def _run_sweep_repeat(self, initial_state: np.ndarray,
+                          circuit: circuits.Circuit,
+                          qubit_order: 'cirq.QubitOrderOrList',
                          repetitions: int) -> Dict[str, np.ndarray]:


Is it worth adding a developer-centric docstring? It's a private method, so probably not mandatory, but might be nice to have.

Done. Also renamed it.

dstrain115 · 2020-06-17T20:44:05Z

cirq/sim/sparse_simulator.py

+
+
+def _split_into_unitary_then_general(circuit: 'cirq.Circuit'
+                                    ) -> Tuple['cirq.Circuit', 'cirq.Circuit']:


Ditto. I would either expand on the comment above "Simulate as many unitary operations as possible..." or write a docstring to explain what this is doing and returning.

Acknowledged. In this case I think the method name is pretty descriptive.

Added a docstring.

dstrain115 · 2020-06-17T20:45:23Z

cirq/sim/sparse_simulator_test.py

@@ -147,7 +147,7 @@ def test_run_invert_mask_measure_not_terminal(dtype):
                np.testing.assert_equal(result.measurements,
                                        {'m': [[1 - b0, b1]] * 3})
                assert result.repetitions == 3
-        assert mock_sim.call_count == 12
+        assert mock_sim.call_count > 4


For this and below, would it be worth a line comment to explain what the assertion is testing and/or why ">4" is expected?

Added comments

dstrain115 · 2020-06-17T20:46:06Z

cirq/sim/sparse_simulator_test.py

+
+
+def test_entangled_reset_does_not_break_randomness():
+    # A previous version made the mistake of assuming that it was okay to cache


Do you think this should use the triple quote doc string instead of the line comments?

Huh. I never considered putting a doc string on a test method.

Made it a doc string.

…ix_sparse_random

…uantumlib#3093) - This resulted in some circuits with random behavior giving long lists of identical samples when repeated - Change the general strategy used by the simulator to one of extracting a unitary prefix that can be cached, and then looking at the rest to decide whether sampling can be done efficiently or not

Strilanc requested review from dabacon and viathor June 16, 2020 00:03

googlebot added the cla: yes Makes googlebot stop complaining. label Jun 16, 2020

Strilanc added 3 commits June 15, 2020 17:05

Merge branch 'master' of github.com:quantumlib/Cirq into fix_sparse_r…

8adfe20

…andom # Conflicts: # cirq/sim/sparse_simulator.py

Ensure consistent order

4829bb8

types

98dfcd6

Strilanc requested a review from dstrain115 June 16, 2020 02:57

dabacon approved these changes Jun 17, 2020

View reviewed changes

dstrain115 reviewed Jun 17, 2020

View reviewed changes

Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 17, 2020

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 17, 2020

Merge branch 'master' into fix_sparse_random

ace299d

Strilanc removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jun 17, 2020

Strilanc added 2 commits June 17, 2020 14:20

Doug comments

c7c5fbc

Merge branch 'fix_sparse_random' of github.com:quantumlib/Cirq into f…

65358b0

…ix_sparse_random

Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 17, 2020

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 17, 2020

CirqBot merged commit 037afff into master Jun 17, 2020

CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 17, 2020

CirqBot deleted the fix_sparse_random branch June 17, 2020 21:31

CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cirq.Simulator assuming a reset is safe to cache before sampling #3093

Fix cirq.Simulator assuming a reset is safe to cache before sampling #3093

Strilanc commented Jun 16, 2020

dabacon left a comment

dabacon Jun 17, 2020

Strilanc Jun 17, 2020

dabacon Jun 17, 2020

Strilanc Jun 17, 2020

dabacon Jun 17, 2020

Strilanc Jun 17, 2020

dstrain115 Jun 17, 2020

Strilanc Jun 17, 2020

dstrain115 Jun 17, 2020

Strilanc Jun 17, 2020

dstrain115 Jun 17, 2020

Strilanc Jun 17, 2020

Strilanc Jun 17, 2020

dstrain115 Jun 17, 2020

Strilanc Jun 17, 2020

dstrain115 Jun 17, 2020

Strilanc Jun 17, 2020

Strilanc Jun 17, 2020



		def _split_into_unitary_then_general(circuit: 'cirq.Circuit'
		) -> Tuple['cirq.Circuit', 'cirq.Circuit']:



		def test_entangled_reset_does_not_break_randomness():
		# A previous version made the mistake of assuming that it was okay to cache

Fix cirq.Simulator assuming a reset is safe to cache before sampling #3093

Fix cirq.Simulator assuming a reset is safe to cache before sampling #3093

Conversation

Strilanc commented Jun 16, 2020

dabacon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment