qiboteam / qibolab

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

Adding flux pulse creation in the platform #771

Closed GabrielePalazzo closed 7 months ago

GabrielePalazzo commented 7 months ago

Checklist:

codecov[bot] commented 7 months ago

Codecov Report

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

Comparison is base (92f35df) 62.45% compared to head (0eebab9) 62.48%. Report is 15 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #771 +/- ## ========================================== + Coverage 62.45% 62.48% +0.03% ========================================== Files 47 47 Lines 5867 5872 +5 ========================================== + Hits 3664 3669 +5 Misses 2203 2203 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/771/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/771/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `62.48% <100.00%> (+0.03%)` | :arrow_up: | 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 7 months ago

@stavros11 since we mentioned the option of dropping all the Platform.create_* methods, should we close this one?

stavros11 commented 7 months ago

@stavros11 since we mentioned the option of dropping all the Platform.create_* methods, should we close this one?

I guess it is still not clear if we will drop (or change) Platform.create_* methods, but in any case this will be discussed as part of the 0.2 refactor. For now, I do not mind even merging this and we can update/remove together with the rest later.

My only concern regarding the new method in particular is that, unlike create_RX_pulse and create_MZ_pulse (the native gates), the new one does not read any information from the platform. Instead the user is supposed to pass everything (duration, amplitude) as arguments. Therefore, I am not sure what we gain by having it under Platform, given that the user is still free to define flux pulses using the pulse API directly (with minimal usability overhead compared to calling this method). For that maybe @GabrielePalazzo has some arguments. I believe it was needed for some routine in qibocal.

alecandido commented 7 months ago

I guess it is still not clear if we will drop (or change) Platform.create_* methods, but in any case this will be discussed as part of the 0.2 refactor. For now, I do not mind even merging this and we can update/remove together with the rest later.

Ok, even fine merging. I would just avoid keeping this in a limbo, and since it was draft it would have been easier to close. But both closing and merging are fine (while keeping it stale is not great...).

GabrielePalazzo commented 7 months ago

Therefore, I am not sure what we gain by having it under Platform, given that the user is still free to define flux pulses using the pulse API directly (with minimal usability overhead compared to calling this method). For that maybe @GabrielePalazzo has some arguments. I believe it was needed for some routine in qibocal.

Yes, the only reason why I added the create_ method here was to avoid defining the exact same pulse in different qibocal routines.