qiboteam / qibolab

Quantum hardware module and drivers for Qibo.
https://qibo.science
Apache License 2.0
43 stars 14 forks source link

Unify execution interface #861

Closed alecandido closed 2 months ago

alecandido commented 6 months ago

Close #755

alecandido commented 6 months ago

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 commented 2 months ago

@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[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/qiboteam/qibolab/pull/861/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qiboteam/qibolab/pull/861/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `44.93% <100.00%> (-0.25%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alecandido commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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)