search

qiboteam / qibolab

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

Unrolling bounds as instrument settings #832

Closed stavros11 closed 3 months ago

stavros11 commented 4 months ago

Implements what is described in https://github.com/qiboteam/qibolab/pull/817#discussion_r1515985368.

codecov[bot] commented 4 months ago

Codecov Report

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

Project coverage is 66.35%. Comparing base (926104e) to head (4946322).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #832 +/- ## ========================================== - Coverage 66.44% 66.35% -0.09% ========================================== Files 50 50 Lines 6079 6063 -16 ========================================== - Hits 4039 4023 -16 Misses 2040 2040 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/832/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/832/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `66.35% <100.00%> (-0.09%)` | :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.

stavros11 commented 3 months ago

Looks good, should we open another PR in qibolab platforms as well ?

I believe this is not necessary. If no bounds are given in parameters.json it will use the defaults hardcoded for the used instrument. These are the values from your PR which I believe are good estimates for what we know so far. If someone finds better values then they can put them in the platform. The only confusion may be that platforms dumped by qibocal will now contain the default bounds (even when not specified in the original platform), but that shouldn't be an issue.

@alecandido I fixed the conflict and responded to your comments. If you have no objection, feel free to merge. Regarding instrument.dump(), maybe we should migrate all instruments to that approach, but certainly not here.

alecandido commented 3 months ago

I manually changed base branch to the PR. @scarrazza is it possible to enable to automated rebase in the settings?

alecandido commented 3 months ago

The only confusion may be that platforms dumped by qibocal will now contain the default bounds (even when not specified in the original platform), but that shouldn't be an issue.

For this we should decide a consistent scheme, since those platforms are becoming lock files, but they should not be recommitted as is to the platforms' repo (otherwise it would not make use of the defaults any longer). It would be nice if we could generate both fully detailed platforms (for reproducibility) and minimal updates (to recommit them back).