Scan values should be run-time option

[nkanazawa1989]

What is the expected behavior?

In the current implementation of T1 experiment, delays is passed to the constructor and becomes an immutable instance variable. This should be runtime option so that we can update scan range without creating new instance.

For me, in the experiment that scans some parameters, it is more preferable if we can check the parametrized circuit rather than list of circuits with assigned parameters. This can be implemented with

self._delay = Parameter('delay')

experiment_circuit = QuantumCircuit(1, 1)
experiment_circuit.x(0)
experiment_circuit.barrier(0)
experiment_circuit.delay(self._delay, 0, self._unit)
experiment_circuit.barrier(0)
experiment_circuit.measure(0, 0)

self._experiment_circuit = experiment_circuit  # we have some property to show this, no setter

and in .circuits method

t1_circuits = []
for delay in delays:
    circ = self._experiment_circuit.assign_parameters({self._delay: delay}, inplace=False)
    circ.metadata = ...
    t1_circuits.append(circ)

This will allow us to check the circuit with parameter.

t1_exp.experiment_circuit.draw()

     ┌───┐ ░ ┌──────────────────┐ ░ ┌─┐
q_0: ┤ X ├─░─┤ DELAY(delay[dt]) ├─░─┤M├
     └───┘ ░ └──────────────────┘ ░ └╥┘
c: 1/════════════════════════════════╩═
                                     0 

[yaelbh]

Thanks @nkanazawa1989. Here are a few thoughts:

1. Could you elaborate about the advantage of not creating a new T1Experiment instance for a new set of delays.
2. The solution of using Parameter will not work if the number of delays has changed.
3. I think Rework experiment options #39 allows you to change the delays also after class initialization. But you would still have to provide an initial set of delays.
4. I'm indeed not sure if delays should be a parameter of the constructor (like now) or of run. I vaguely remember that the current implementation is a result of some discussion that took place. But, since I don't remember the details of that discussion (if it really happened) then we should reconsider.

[nkanazawa1989]

Could you elaborate about the advantage of not creating a new T1Experiment instance for a new set of delays.

See this example

t1exp = T1Experiment(qubit=0)
exp_data = t1exp.run(backend, delays=np.linspace(0, 100e-6, 10))
exp_data.analysis_result(0) # check the result plot, fit is poor due to wrong initial guess of scan range

# add other data points
exp_data = t1exp.run(backend, delays=np.linspace(0, 10e-6, 10), experiment_data=exp_data)

If scan values are runtime option, such workflow can be written more smoothly.

I think #39 allows you to change the delays also after class initialization. But you would still have to provide an initial set of delays.

Even if we upgrade the experiment option with #39,

t1exp.delays

doesn't make sense. If we run the second scan in above example, the delays that exp_data has should be np.concatenate((np.linspace(0, 100e-6, 10), np.linspace(0, 10e-6, 10))). However, this doesn't work because self.delays should represent current set of delays to execute. Thus I think scan values should be runtime value rather than the instance variable.

The solution of using Parameter will not work if the number of delays has changed.

I think you misunderstand the parameter. Parametrized circuit defines a template program (just a single entry), and you can assign arbitrary numbers of parameters later on, by calling assign_parameters method.

I vaguely remember that the current implementation is a result of some discussion that took place

I didn't carefully follow the discussion of T1 PR. If there is any reasonable reason, I'm fine with current implementation.

[yaelbh]

@nkanazawa1989 It makes sense to me. Additional opinions will be helpful, in case that we miss something. @coruscating @wshanks

[yaelbh]

@nkanazawa1989 Is it relevant to other experiments as well? For example I've just noticed that #31 has the same structure, and actually I guess that it's the same for all the experiments that we did until now.

[wshanks]

I have not worked with the experiments API enough to have a strong opinion about run() vs. __init__(). I do dislike the fact that there is this split between the two methods where you need to track which parameters go where. My preference (in the abstract without running many experiments) is to pass everything to __init__() and then call run() with no arguments to avoid the need to track parameters between the two methods. Allowing parameters to be overridden in run() seems fine to me though. So for T1 this would mean __init__ could take delays and those values could be used by default but passing delays= to run() could override them.

@nkanazawa1989's example of visualizing a parametrized circuit is a good argument for preferring a parametrized template circuit over generating a set of completed circuits. Not all experiments fit the format of a scanning one parameter of a single parametrized circuit but a lot of them do. Should this be standardized in some way like a subclass for scanned experiments?

Right now T1 does not override the run() method and even #39 does not provide support for passing an option from run() to circuits(). Should it? @chriseclectic

That said, I think I would just run a new T1 experiment if I wanted a different range 🙂 I think we have said we want to support taking additional data in general though.

[nkanazawa1989]

@yaelbh Yes, this is not only the problem of T1 but also for all other experiments doing scan. We should define a standard now.

@wshanks Thanks. I'm okey with having delays both in the constructor and run option. Basically we don't want to create another instance just to change the scan range.

[yaelbh]

#65 is relevant here

[yaelbh]

I'll reconsider this issue, hopefully soon

[nkanazawa1989]

I think currently most of classes conform to this behavior

class Experiment(BaseExperiment):
    def __init__(qubits, my_scan_values):
         super().__init__(qubits)
         self.set_experiment_options(scan_values=my_scan_values)

Probably removing something like self.scan_values = my_scan_values from all experiment constructor is enough.

[yaelbh]

Do you think it's important to convert T1 circuits into a single parametrized circuit?

[yaelbh]

Closing because the current implementation seems to be ok