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

Modify qrm_rf driver so that qrm_rf modules can be used as qcm_rf to generate drive pulses. #800

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

aorgazf
Copy link
Member

@aorgazf aorgazf commented Feb 13, 2024

The driver of the qrm_rf is modified so that if the qrm_rf does not receive any readout pulses, it does not trigger any acquisitions, allowing the use of the module for drive only.

The cluster connected to tii1q_d28 has 2 qrm_rf modules, due to the lack of acm_rf modules.

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@aorgazf aorgazf added the enhancement New feature or request label Feb 13, 2024
Copy link

codecov bot commented Feb 13, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (758e9a1) 63.89% compared to head (97c8b63) 63.88%.
Report is 6 commits behind head on main.

❗ Current head 97c8b63 differs from pull request most recent head 78f9d4f. Consider uploading reports for the commit 78f9d4f to get more accurate results

Files Patch % Lines
src/qibolab/instruments/qblox/cluster_qrm_rf.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #800      +/-   ##
==========================================
- Coverage   63.89%   63.88%   -0.02%     
==========================================
  Files          49       49              
  Lines        5792     5793       +1     
==========================================
  Hits         3701     3701              
- Misses       2091     2092       +1     
Flag Coverage Δ
unittests 63.88% <0.00%> (-0.02%) ⬇️

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

@PiergiorgioButtarini PiergiorgioButtarini left a comment

Choose a reason for hiding this comment

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

Thanks @aorgazf!

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

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

Thanks. It is a minimal change so no objections in terms of code, @PiergiorgioButtarini feel free to merge if this solves the issue @biankawolo is having.

src/qibolab/instruments/qblox/cluster_qrm_rf.py Outdated Show resolved Hide resolved
@alecandido
Copy link
Member

@PiergiorgioButtarini: in principle, we could make a minimal subclass making it clear that it is a qrm_rf used for this purpose.

However, I'd postpone after the Module reorganization, when the main class itself will be as minimal as possible.
In case you agree with the plan, just open an issue about this.

@scarrazza scarrazza added this to the Qibolab 0.1.6 milestone Feb 15, 2024
@PiergiorgioButtarini
Copy link
Contributor

@alecandido yes we could but idk how much this will be really useful in the future. Consider that using the qrm as a drive module is a necessity due to the temporary lack of QcmRf and in the next weeks we should receive an order that will avoid this problem.
If you think that in general is an interesting feature to have, I will open an issue about it.

@alecandido
Copy link
Member

alecandido commented Feb 15, 2024

I believe it is not a bad idea in general. Of course, it is better to use the qrm for what is made for, since it would be a waste in the long term.

But this setup may happen, maybe even not only in the TII lab (provided that someone else is using Qibolab), as it happened for you know.
Given that it requires so little to support it, I would keep it as a feature.

@PiergiorgioButtarini
Copy link
Contributor

Yes if we consider that other lab can have the same problem it might be a nice feature to have. Thanks I will open an issue and merge this!

Co-authored-by: Stavros Efthymiou <35475381+stavros11@users.noreply.github.com>
@PiergiorgioButtarini PiergiorgioButtarini merged commit e2ecafa into main Feb 15, 2024
22 checks passed
@alecandido alecandido deleted the alvaro/qrm_as_qcm_fix branch February 28, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants