Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Apr 30, 2024
Merged

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

merged 1 commit into from
Apr 30, 2024

Conversation

stavros11
Copy link
Member

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

Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #888   +/-   ##
=======================================
  Coverage   66.59%   66.59%           
=======================================
  Files          55       55           
  Lines        5942     5942           
=======================================
  Hits         3957     3957           
  Misses       1985     1985           
Flag Coverage Δ
unittests 66.59% <ø> (ø)

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.

Copy link
Contributor

@hay-k hay-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct to me.

Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alecandido
Copy link
Member

Ok, in 0.2 it was already correct

def q(self, samples: int) -> Waveform:
"""Generate ...
.. todo::
"""
ts = np.arange(samples)
mu = (samples - 1) / 2
sigma = _samples_sigma(self.rel_sigma, samples)
return self.beta * (-(ts - mu) / (sigma**2)) * self.i(samples)

Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alecandido
Copy link
Member

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
Copy link
Member Author

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.

@stavros11 stavros11 merged commit 483c5a5 into main Apr 30, 2024
33 checks passed
@stavros11 stavros11 deleted the fixdragq branch April 30, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants