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

Enable raw acquisition for rfsoc #480

Merged
merged 6 commits into from
Jun 12, 2023
Merged

Conversation

rodolfocarobene
Copy link
Contributor

As title says, this PR should implement raw acquisition (for pulse sequences) for the rfsocs.
It should be ok, but I'm waiting to test it.

Checklist:

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

@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Patch coverage: 42.85% and project coverage change: -0.08 ⚠️

Comparison is base (088733d) 59.32% compared to head (0da1807) 59.24%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #480      +/-   ##
==========================================
- Coverage   59.32%   59.24%   -0.08%     
==========================================
  Files          37       37              
  Lines        6203     6211       +8     
==========================================
  Hits         3680     3680              
- Misses       2523     2531       +8     
Flag Coverage Δ
unittests 59.24% <42.85%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/qibolab/instruments/rfsoc.py 55.21% <42.85%> (-1.76%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rodolfocarobene rodolfocarobene added deploy on hardware The PR needs to be tested on hardware before merging and removed deploy on hardware The PR needs to be tested on hardware before merging labels Jun 9, 2023
@rodolfocarobene rodolfocarobene marked this pull request as ready for review June 9, 2023 08:29
@rodolfocarobene
Copy link
Contributor Author

This PR is ready, but there are a few things to note:

  • coverage does decrease, but I'm addressing the problem of low coverage (that at the beginning was caused by the server) in a different PR (Time sweepers rfsoc #474) so I would fix this there.
  • I tested the routine on hardware using a loopback configuration, connecting directly readout and feedback. Eventually the new feature can be tested using Add optimal integration weights routine qibocal#383 (in particular the tof routine)
  • At the moment I didn't remove the changes like Dict->dict
  • This should be merged after the qibosoq one

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.

Apart from the discussion on new-style type hints, this PR seems good to go.

Comment on lines +327 to +330
if len(sequence.ro_pulses) != 1:
raise NotImplementedError("Raw data acquisition is compatible only with a single readout")
if execution_parameters.averaging_mode is not AveragingMode.CYCLIC:
raise NotImplementedError("Raw data acquisition can only be averaged")
Copy link
Contributor

@Jacfomg Jacfomg Jun 9, 2023

Choose a reason for hiding this comment

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

Have you tested if it only allows a single waveform to be acquired ? Maybe it is useful if 2 fit in memory for readout crosstalk analysis ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Jacfomg, you are right and I'll try!
I could probably add something more sofisticated than just counting the readout pulses... In fact also single readout pulse, very long, could fill the memory...
In any case, I think all of these are just general improvements and not really required in the short term

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 for implementing this. If you want to merge this now, you can transfer the discussion about multiple waveform acquisition to an issue (if you are planning to look at it later).

FYI, with QM could also not acquire shots of raw waveforms (AveragingMode.SINGLESHOT doesn't work with AcquisitionType.RAW), so for nshots > 1 I could only get the averaged waveform. However, I don't remember for sure, but I believe I could get multiple waveforms by playing multiple readout pulses (on different qubits) at the same time.

pyproject.toml Outdated Show resolved Hide resolved
@rodolfocarobene rodolfocarobene merged commit 4c4c092 into main Jun 12, 2023
@rodolfocarobene rodolfocarobene deleted the rfsoc-raw-acquisition branch June 12, 2023 05:33
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.

5 participants