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

control_panel - Fix rounding error that caused voltage drifts #211

Merged
merged 8 commits into from
May 21, 2024

Conversation

matpompili
Copy link
Contributor

A rounding error in the voltage book-keeping causes voltage drifts when switching in between two voltages. An example program switches between -0.05V and 0.05V (factor of two in the scope is because of missing 50Ω termination).

Essentially, every qua.play instruction is missing one unit of DAC voltage (2^-16), so on repeated calls the voltage drifts.
This PR fixes the error. It also replaces np.round with round as it is a factor of ~30 faster.

Before the fix:
image

After the fix:
image

Remember to:

  • Update the CHANGELOG.md
  • Added tests for the feature or fix

@yomach
Copy link
Collaborator

yomach commented May 9, 2024

Hi @matpompili,
Thanks!
I'm slightly confused by this. Following the math, I'm failing to see the issue.
If I do:
round(-0.05 / 2**-16) * 2**-16 + round(0.05 / 2**-16) * 2**-16, which is applying your voltages with the current rounding, then I get 0.
If I apply your fix, I get something else (two LSB units, 2*2**-16)
I will check on our simulator

@yomach
Copy link
Collaborator

yomach commented May 16, 2024

Hi @matpompili
After some debugging, the issue was not taking into effect the rounding (down) that is happening on the Pulse Processor. I've pushed a commit to your branch that fixes it. It seems to solve the issue.

Your fix, while worked in this case, might produce drift in other cases.

Can you please test it as well?

@matpompili
Copy link
Contributor Author

Thanks @yomach it works!

I made a couple of small edits:

  • Removed the if statement by making two functions
  • Converted np calls to math since they are faster on scalars.

Tested on setup, good to merge from our end!

@yomach yomach merged commit 798e44d into qua-platform:main May 21, 2024
1 check passed
yomach added a commit that referenced this pull request Jun 9, 2024
* Fix voltage booking error introduced in the previous fix.

* Change variables to be explicit

* remove special 0 case for ramp_to_zero

---------

Co-authored-by: Yoav Romach <[email protected]>
Co-authored-by: Yoav Romach <[email protected]>
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.

2 participants