search

qiboteam / qibolab

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

Bug(?) in q-component of DRAG pulse #888

Closed stavros11 closed 5 months ago

stavros11 commented 5 months ago

Thanks @sorewachigauyo for reporting this. It would be could if someone else can confirm this is correct because I am not 100% sure.

codecov[bot] commented 5 months ago

Codecov Report

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

Project coverage is 66.59%. Comparing base (76def93) to head (ad50c8c).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #888 +/- ## ======================================= Coverage 66.59% 66.59% ======================================= Files 55 55 Lines 5942 5942 ======================================= Hits 3957 3957 Misses 1985 1985 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/888/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/888/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `66.59% <ø> (ø)` | | 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 5 months ago

Ok, in 0.2 it was already correct https://github.com/qiboteam/qibolab/blob/f829e8f02e54abcd1fc18de5e2d1cd1df367d0a7/src/qibolab/pulses/envelope.py#L186-L194

alecandido commented 5 months ago

It makes sense. I didn't notice any difference when calibrating with main and this branch which seems weird...

In a few relevant cases, sampling_rate will be just 1, so it's not incredibly strange

stavros11 commented 5 months ago

Thank you all for checking. This already has several approvals so I will merge.

I should check if this is still present in 0.2 as well.

I also checked there for a second "opinion" and saw that it was fixed.

It makes sense. I didn't notice any difference when calibrating with main and this branch which seems weird...

In a few relevant cases, sampling_rate will be just 1, so it's not incredibly strange

Indeed, for QM and QBlox sampling_rate is 1 so this shouldn't make any difference. That may also be why it was not noticed for so long.