Unify execution interface

[alecandido]

Close #755

• propagate to Controller.play()
• drop Controller.sweep()
• fix tests

[alecandido]

Currently, it is at the stage of just an interface proposal, but it would require to propagate the sweeper usage to all the Controller.play() definitions, and drop the .sweep() (on top of fixing tests).

[alecandido]

@stavros11 @hay-k the biggest change is (of course) in qibolab.platform.platform, where the previous methods have been replaced by the unique Platform.execute(), and in qibolab.instruments.abstract, where Controller.sweep() has been suppressed.

Everything else is just a matter of propagating these changes and fixing the tests.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.93%. Comparing base (673425d) to head (79b5c14).

Additional details and impacted files

@@            Coverage Diff             @@
##              0.2     #861      +/-   ##
==========================================
- Coverage   45.17%   44.93%   -0.25%     
==========================================
  Files          70       70              
  Lines        6255     6224      -31     
==========================================
- Hits         2826     2797      -29     
+ Misses       3429     3427       -2     

Flag	Coverage Δ
unittests	44.93% <100.00%> (-0.25%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stavros11]

Thanks @alecandido. Looks good to me and I think the faster we merge this, the better for other ongoing developers (drivers).

[stavros11]

Maybe in this docstring, most likely in the Returns: part (but I cannot comment directly there), we could add a more detailed sentence about what exactly is returned. For example a dict from readout pulse ids to ExecuteResult objects.

That reminds me that at some point we need to finalize the output of this function for 0.2, since this is also user facing. Certainly not for this PR, but currently, I am not sure about the following:

• Should we keep the dict and with what keys? Currently we are using serial (replaced by id) and also qubit name. Personally, I think we should keep the dict but only with ids, not qubits.
• Should we keep separate objects for different averaging/acquisition modes, or just replace everything with array?
• Unrolling: I think id is still sufficient and we don't need lists, if all readouts in the given sequences have distinct ids.
• Sweepers: I think here the same id is associated to multiple measurements so maybe we need multi-dimensional arrays (as we have now), however I could think some complicated cases (eg. we are sweeping something on the readout pulse).

[alecandido]

Should we keep the dict and with what keys? Currently we are using serial (replaced by id) and also qubit name. Personally, I think we should keep the dict but only with ids, not qubits.

Agreed

Should we keep separate objects for different averaging/acquisition modes, or just replace everything with array?

Replace with arrays. #809 #899 are both listed for 0.2

Unrolling: I think id is still sufficient and we don't need lists, if all readouts in the given sequences have distinct ids.

That is fine by me. It would keep the return object flat (less nested)

Sweepers: I think here the same id is associated to multiple measurements so maybe we need multi-dimensional arrays (as we have now), however I could think some complicated cases (eg. we are sweeping something on the readout pulse).

Sweepers are for loops, by definition. We should collect them within multidimensional arrays, since each iteration is homogeneous with the others.
Possibly, we should identify results with just the id of the original pulse, if newer readout pulses are introduced in the process. But this should be fully internal to the drivers, despite non-trivial

[stavros11]

Following the above comment, why do we need to index using [0] here? This is probably a patch for tests to pass, however do we actually want the results to be a dict with list values now?

[alecandido]

This is just a patch, let's provide a better solution with #809 and #899

[alecandido]

@stavros11 thanks for your comments: I would avoid implementing both, even the docstring, because the current result is not here to stay.

Current patches are there because it's useful to have tests passing, even for a work-in-progress 0.2 version, but they will also change as a consequence of new developments concerning the results.

I would close this as it is, unless you or @hay-k have any other comments.
To move forward, if no other feedback will be collected, I will merge on Monday/Tuesday (in this case approvals are only informal, since we're not merging to main), and then start working asap on #809 and #899. @stavros11 if you can provide some support, we could even prototype the results' generation in QM. I will start defining the interface and implementing for dummy.

[stavros11]

I would close this as it is, unless you or @hay-k have any other comments. To move forward, if no other feedback will be collected, I will merge on Monday/Tuesday (in this case approvals are only informal, since we're not merging to main), and then start working asap on #809 and #899.

No comments from my side, I agree with the plan and merging this as it is. I also agree with the summary in the issues.

@stavros11 if you can provide some support, we could even prototype the results' generation in QM. I will start defining the interface and implementing for dummy.

Sure, when you define the interface I will propagate it to QM.

[alecandido]

As previously announced, I'm merging this, and keep going with #809 and #899.

Thanks @stavros11 for your review. I will keep into account in those issues (I'm going to directly cite it in there)