-
Notifications
You must be signed in to change notification settings - Fork 14
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
Initial implementation for Keysight QCS #944
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #944 +/- ##
==========================================
- Coverage 51.95% 48.81% -3.15%
==========================================
Files 63 69 +6
Lines 2808 2989 +181
==========================================
Hits 1459 1459
- Misses 1349 1530 +181
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
3de5459
to
1a4d052
Compare
@sorewachigauyo I previously rebased https://github.com/qiboteam/qibolab/tree/0.2 on current https://github.com/qiboteam/qibolab/tree/main, thus I also rebased your PR consequently. The Rust workflow is failing for unknown reasons (nothing changed on our side, so the reason should be in some dependency, but it seems non-reproducible locally). P.S.: same for QuTiP, which is making the other workflows to fail as well (but we should merge the PR very soon) |
c14a227
to
41dd538
Compare
Initial testing with readout spectroscopy using runcard https://github.com/qiboteam/qibolab_platforms_nqch/tree/qibolab-0.2/iqm5q import numpy as np
import matplotlib.pyplot as plt
from qibolab import (
AcquisitionType,
AveragingMode,
Parameter,
Sweeper,
create_platform,
)
platform = create_platform("iqm5q")
platform.connect()
qubit = platform.qubits[4]
natives = platform.natives.single_qubit[4]
sequence = natives.MZ.create_sequence()
f0 = platform.config(qubit.probe).frequency # center frequency
sweeper = Sweeper(
parameter=Parameter.frequency,
range=(f0 - 20e6, f0 + 100e6, 1e6),
channels=[qubit.probe],
)
results = platform.execute([sequence],
[[sweeper]],
nshots=100,
relaxation_time=100e3,
averaging_mode=AveragingMode.SEQUENTIAL,
acquisition_type=AcquisitionType.INTEGRATION)
_, acq = next(iter(sequence.acquisitions))
# plot the results
signal = results[acq.id]
amplitudes = np.abs(signal[..., 0] + 1j * signal[..., 1])
frequencies = sweeper.values
plt.title("Resonator Spectroscopy")
plt.xlabel("Frequencies [GHz]")
plt.ylabel("Amplitudes [a.u.]")
plt.plot(frequencies / 1e9, amplitudes * 1e6)
plt.savefig("test.png") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the lengthy review 😅
src/qibolab/_core/instruments/qcs.py
Outdated
for channel_id, input_op in acquisitions: | ||
channel = self.virtual_channel_map[channel_id] | ||
if channel not in acquisition_map: | ||
acquisition_map[channel] = [] | ||
acquisition_map[channel].append(input_op) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, we could even lift the construction of such a mapping to the PulseSequence
itself (do you think it may be useful elsewhere? @stavros11)
Of course, it would be a ChannelId -> [InputOps]
mapping, that you could supplement with your ChannelId -> qcs.Channels
mapping to obtain the desired result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sorewachigauyo, now it seems extremely cleaner.
I'm sorry that a bunch of functions right now require a lot of arguments. We could try to find a better way to split some parts, partially evaluate tasks, or better package the arguments.
But this is really the kind of refactor that can be postponed (almost forever...).
Most of the comments are on the sweepers, which is frequently the most critical part. All the others seem pretty nice (though I may spend a bit more time later on proposing a small refactor about the array manipulations in the result - but nothing that serious, it's already pretty succinct).
for idx2, sweeper in enumerate(parallel_sweeper): | ||
qcs_variable = qcs.Scalar( | ||
name=f"V{idx}_{idx2}", value=sweeper.values[0], dtype=float | ||
) | ||
|
||
if sweeper.parameter is Parameter.frequency: | ||
sweeper_channel_map.update( | ||
{channel_id: qcs_variable for channel_id in sweeper.channels} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @sorewachigauyo, this is not about this PR, but rather Qibolab's overall design. I'm writing this here because it is an optimal example to discuss, and you're invited to provide your opinion as well. I may lift this immediately as a separate issue, I'm just undecided.
With @stavros11, we wondered at some point whether to strip away the option of having multiple channels and pulses swept by a single sweeper (not a parallel one). In fact, this could be now replaced by an equivalent set of parallel sweepers.
We didn't do that because of the specific implementation of parallel sweepers in QM, which requires the usage of a for_each_
context manager, which had some disadvantages with respect to a plain for_
.
It now appears to me that quite redundant, since here you have explicitly three nested iterations:
idx
about the sweepers' groupidx2
within the parallel sweepers- and
sweeper.channels
(orsweeper.pulses
down below - exact analogue)
These could have been decreased by one level by unraveling the .channels
in the parallel group.
However, it is true that, thanks to the presence of multiple .channels
, you are able to use a single qcs_variable
, and this could be an advantage, and a reason to keep the current behavior (which we would keep anyhow, but we may consider deprecating at some point - and finally removing in 0.3).
What do you think?
I like that as well, e.g. the strictly modular structure in Rust, in which you need to declare any visibility beyond the current namespace. However, that's not Python style, and even The Python philosophy is to leave everything public, since
(not able to find a proper reference, sorry, but it is a GvR quote) The only way to go beyond that are naming conventions, and I'd suggest avoiding double leading underscores, unless you're extremely confident (but it's unfamiliar and a bit confusing...). So, whatever is inside |
9b5f6f8
to
11ba3cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another tiny proposal for the results handling, but other than that it looks good to me.
I'll wait for @stavros11 review, but it may be good to go :)
Thanks @sorewachigauyo for your effort!
Trial implementation of the core functions. Testing is rough on my side since I do not currently have access to the instrument library on my development machine, so I have to test the code on the embedded controller.
Checklist: