Reduce required boilerplate for implementing external simulators and samplers

[cduck]

I wrote a custom simulator recently and I had to copy quite a bit of boilerplate code out of cirq/sim/sparse_simulator.py. Below is my code with the boilerplate pointed out. It would be nice if the code in SimulatesSamples and SimulatesIntermediateState parent classes were rearranged to reduce this. @Strilanc

class MySimulator(cirq.SimulatesSamples,
                  cirq.SimulatesIntermediateState):
    # Boilerplate
    def _run(self, circuit, param_resolver, repetitions):
        param_resolver = param_resolver or cirq.ParamResolver({})
        resolved_circuit = cirq.resolve_parameters(circuit, param_resolver)
        assert not cirq.is_parameterized(resolved_circuit)
        return self._run_sweep_repeat(resolved_circuit, repetitions)

    # Boilerplate
    def _run_sweep_repeat(self, circuit, repetitions):
        measurements = defaultdict(list)
        for _ in range(repetitions):
            all_step_results = self._base_iterator(
                circuit,
                qubit_order=cirq.QubitOrder.DEFAULT,
                initial_state=0)
            for step_result in all_step_results:
                for k, v in step_result.measurements.items():
                    measurements[k].append(np.array(v, dtype=np.uint8))
        return {k: np.array(v, dtype=np.uint8) for k, v in measurements.items()}

    # Boilerplate
    def _simulator_iterator(self, circuit, param_resolver, qubit_order,
                            initial_state):
        param_resolver = param_resolver or cirq.ParamResolver({})
        resolved_circuit = cirq.resolve_parameters(circuit, param_resolver)
        assert not cirq.is_parameterized(resolved_circuit)
        actual_initial_state = 0 if initial_state is None else initial_state
        return self._base_iterator(resolved_circuit, qubit_order,
                                   actual_initial_state,
                                   perform_measurements=True)

    def _base_iterator(self, circuit, qubit_order, initial_state,
                       perform_measurements=True):
        # Boilerplate
        qubits = cirq.QubitOrder.as_qubit_order(qubit_order).order_for(
            circuit.all_qubits())
        num_qubits = len(qubits)
        qid_shape = cirq.qid_shape(qubits)
        qubit_map = {q: i for i, q in enumerate(qubits)}
        if len(circuit) == 0:
            yield MySimulatorStep(<default state>, {}, qubit_map)
        def on_stuck(bad_op):
            return TypeError(
                "Can't simulate unknown operations that don't specify a"
                "unitary or a decomposition. {!r}".format(bad_op))
        def keep(potential_op):
            return (cirq.has_unitary(potential_op) or
                    cirq.has_mixture(potential_op) or
                    cirq.is_measurement(potential_op) or
                    cirq.op_gate_isinstance(potential_op, cirq.ResetChannel))

        simulator_state = ...  # New code
        for moment in circuit:
            measurements = defaultdict(list)
            known_ops = cirq.decompose(moment, keep=keep,
                                       on_stuck_raise=on_stuck)
            for op in known_ops:
                self.simulate_op(op, simulator_state)  # New code
            yield MySimulatorStep(simulator_state, measurements, qubit_map)

    def simulate_op(op, state):
        # Actual simulation code here
        ...


class MySimulatorStep(cirq.StepResult):
    # Boilerplate
    def __init__(self, state, measurements, qubit_map):
        super().__init__(measurements)
        self.state = state
        self.qubit_map = qubit_map

    # Boilerplate
    def _simulator_state(self):
        return self.state

    def sample(self, qubits, repetitions=1):
        # Actual sampling code here
        return ...

[Strilanc]

Blergh.

Basically every single parameter except the circuit can be handled via glue code inside SimulatesIntermediateState or similar. Qubit order? Transpose results on their way out. Parameter resolver? Resolve the circuit before passing it along. Repetition? For loop.

The only thing the implementer should have to write is def _iter_wavefunctions(self, circuit) or def _final_wavefunction(self, circuit) or something like that.

[daxfohl]

Is anyone working on this? It appears simulator has changed a bit since the issue was reported. Are there still improvements that can be made here? I may be interested if there is reasonable ROI. Presumably this is big enough to require a design first?

[daxfohl]

@cduck @Strilanc I submitted a PR that is lowest-hanging fruit here. The iterator code and a function check_all_resolved was copied verbatim between the simulator subclasses. In the _run code, only the first two lines were identical, and seemed not worth pulling out.

I didn't see anything else that would push down cleanly. There might be some higher-level repetition that I'm overlooking though. If so, I'd like to do that too, though maybe a separate PR since it'd be working at a different level of abstraction. LMK what you think.

[daxfohl]

PR is open for it, but I'm having second thoughts. I pushed _base_iterator down into the base class so that it could be shared. That reduces duplication, but the problem is we can't just get rid of _simulator_iterator because of backwards compatibility. So now there's both _simulator_iterator and _base_iterator, which may end up creating more confusion than it's worth.

What do you think of starting to move toward an approach that decouples the simulation execution from the simulation strategy, get away from the multiple inheritance approach? So what I'm picturing is a static class SimulationExecutor that has methods like run(circuit, simulation_strategy), and the various simulation strategies would implement an interface that the executor would use? This decoupling should allow the simulators to have the bare minimum logic that differentiates them. I'll have to dig more to get a more detailed design, but does the idea sound okay?

It would be a v2 simulation API. I don't think there's a good way to integrate it with v1, but they should be able to exist side by side. And porting a simulator from one to the other should be straightforward. But I don't know, do we want to do that.

[daxfohl]

Pushed PR. Open item remaining to remove/inline deprecated function after v0.11.

The _run boilerplate seems to no longer be boilerplate, as other simulators have implemented it differently. However we may still want to revisit this in a v2 api because the feeling is there is still "similar functionality" that could be extracted, but would be a bigger project.

[tonybruguier]

Hi,

I'm not sure if it's relevant, but there may have been a race condition with #3630 which had just added a new simulator. Sorry, I didn't know that you were working on this and either way, I don't think anything is broken (or even that there's anything issue at all).

Tony

[daxfohl]

@tonybruguier everything should still be backward compatible. Overriding _simulator_iterator still works, is just marked deprecated in favor of overriding _base_iterator. (Everyone's _simulator_iterator code was identical so we pulled it into the base class.)

The only issue I found in any existing simulator was a single unit test that counted compiler warnings, which changed due to the new deprecation warning. All else should continue to work as it was.