Refactor IcarusQ drivers #661

sorewachigauyo · 2023-11-15T02:46:08Z

To update the drivers in line with IcarusQ version alpha.22, which has been stable since July

Checklist:

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

…sq-dev

codecov · 2023-11-15T02:46:28Z

Codecov Report

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

Comparison is base (92f35df) 62.45% compared to head (4a0be6f) 63.78%.
Report is 47 commits behind head on main.

Files	Patch %	Lines
src/qibolab/instruments/icarusqfpga.py	0.00%	180 Missing ⚠️
src/qibolab/instruments/icarusq.py	0.00%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #661      +/-   ##
==========================================
+ Coverage   62.45%   63.78%   +1.33%     
==========================================
  Files          47       47              
  Lines        5867     5744     -123     
==========================================
  Hits         3664     3664              
+ Misses       2203     2080     -123

Flag	Coverage Δ
unittests	`63.78% <0.00%> (+1.33%)`	⬆️

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.

scarrazza · 2023-11-15T09:33:51Z

Thanks @sorewachigauyo, could you please upload the respective create file in https://github.com/qiboteam/qibolab_platforms_nqch?

sorewachigauyo · 2023-11-16T04:26:02Z

Hi @scarrazza, I included the platform and runcard files for the 5Q chip. I'll need to update the create file a bit to work with the new serialize API, as well as update the other chip files for the newer Platform init.

stavros11 · 2023-11-16T12:48:24Z

Hi @scarrazza, I included the platform and runcard files for the 5Q chip. I'll need to update the create file a bit to work with the new serialize API, as well as update the other chip files for the newer Platform init.

Thanks @sorewachigauyo. Let us know if you have any issues while doing this, or feedback that could make the interface easier to use.

sorewachigauyo · 2023-11-17T03:39:10Z

Hi @stavros11,

I think the main issue was the channel mapping. While I was looking at the QRC repo, I noticed that a lot of the channel setup is hardcoded, which may be a problem for readability and with increasing number of channels/qubits. In our case, I tried to make the channel setup as straightforward as possible. I feel that some of these configurations can be moved to the yaml file.

stavros11 · 2023-11-17T14:26:34Z

Thank you @sorewachigauyo.

Regarding the distinction between the create method and the yaml, our idea is that the yaml contains parameters that may change during calibration, while create is static and reflects the lab configuration. The channel configuration is static, so we put it in the create. We also wanted to provide a more programmatic way of defining platforms and keep the yaml only for parameter storage (potentially migrating to something more scalable in the future).

Regarding the channel creation, we tried to improve using the ChannelMap, for example

channels |= ("x1", "tc31", "x2", "tc32")

is a shortcut for creating multiple Channels, however this is probably not very useful as you still need to define the ports. Regardless, the main motivation of defining the channels in Python is that you can use for loops (as you are already doing), which is more scalable than listing everything in a yaml. In your platform https://github.com/qiboteam/qibolab_platforms_nqch/blob/main/icarusq_iqm_5q.py, you can immediately code the patterns without going through dictionaries. For example

for i, qubit in enumerate(qubits.values()):
    qubit.drive = cmap[f"x{i + 1}"]
    qubit.readout = cmap["r1"]

could replace the cmap_qubits dictionary, and similar idea could work (for the most part) for the other dictionaries.

Other than that, the platform looks good to me. One other minor comment is that you can move the instrument settings (controller.setup and attenuator.setup) to the yaml, as the setup is already called when you use load_instrument_settings.

src/qibolab/instruments/icarusq.py

src/qibolab/instruments/icarusqfpga.py

…sq-dev

alecandido · 2024-01-11T12:31:31Z

@sorewachigauyo if there are comments that are old or implemented, please "Resolve the conversation" for them.

If you wish, add a closing message with just something like:

"postponed"
"not relevant"
4f2e6b6 (i.e. the hash of the commit that applies the suggestion)

alecandido

Sorry for the large amount of comments, mostly are trivial or there because I'm not understanding something myself.

I'm still finishing to review the play and sweep methods of the RFSOC_RO.

src/qibolab/instruments/icarusq.py

alecandido · 2024-01-11T12:35:52Z

src/qibolab/instruments/icarusq.py

+    def setup(self):
+        pass


I would say that even .setup() is not mandatory after #739, but we kept it in the interface.

I believe it should be called during platform creation, so it's not required by every platform, unless they use it. But @stavros11 can quickly confirm.

Indeed, instrument.setup() is used by

qibolab/src/qibolab/serialize.py

Line 101 in 9745314

def load_instrument_settings(

to load the instrument settings from the runcard YAML. For this to work you have to explicitly call load_instrument_settings in the platform create method. Note that you don't have access to the physical instruments during platform creation, because platform.connect() is called after that. Therefore, following this approach requires caching the instrument settings in Python and uploading them only during connection.

If there are no settings relevant to the instrument, you can skip this step. setup() still needs to be defined even as empty though, because it is an @abstractmethod of the abstract Instrument. Maybe we could relax this condition @alecandido?

If there are no settings relevant to the instrument, you can skip this step. setup() still needs to be defined even as empty though, because it is an @abstractmethod of the abstract Instrument. Maybe we could relax this condition @alecandido?

You're right, I really forgot.
Thus, I'd keep like it this in this PR, and then relax the constraint and revisit.

src/qibolab/instruments/icarusq.py

src/qibolab/instruments/icarusqfpga.py

alecandido · 2024-01-11T15:39:25Z

src/qibolab/instruments/icarusqfpga.py

+class RFSOC_RO(RFSOC):
+    """IcarusQ RFSoC attached with readout capability."""


Why RFSOC is handling ADCs and READOUT pulses as well, if in general has not readout capabilities?

This is due to the board having both capabilities, but because we use much more DACs than ADCs, in a multiboard scenario, we will not use ADCs on one board.

I will eventually migrate to a cluster approach similar to what was done for Qblox.

Sorry, my comment has been confusing.

I didn't want to sponsor the Qblox approach, not from the API point of view, and not even for the hardware. I'm not even against, simply my message was not about that.
Instead, I like the API to be as transparent as possible, and reflect the hardware, since Qibolab is already another layer on top.

Instead, my point was not much about why RFSOC_RO is doing both (which is fine), but why ADC is handled half in RFSOC_RO and half in the RFSOC base class.
I was highlighting that the distinction between the two is in the _RO version having readout capabilities, but even the base class (which then should not have ro capabilities in general) is handling part of the readout.

src/qibolab/instruments/icarusqfpga.py

alecandido

Ok, I definitely finished my review (and I exaggerated with the comments, sorry...).

However, I'd like to have addressed some of the most relevant comments (or at least to discuss them), but most of them concern style, so they are not required.

Essentially, once test are passing, and it is confirmed to run on hardware (at least the basic functionalities) it's pretty good to go.

src/qibolab/instruments/icarusqfpga.py

alecandido · 2024-01-11T17:01:25Z

src/qibolab/instruments/icarusqfpga.py

+            raw_signal = adc_raw_data[adc]
+            sin = np.sin(2 * np.pi * readout_pulse.frequency * t)
+            cos = np.sin(2 * np.pi * readout_pulse.frequency * t)
+
+            I = np.dot(raw_signal, cos)
+            Q = np.dot(raw_signal, sin)
+            results[readout_pulse.qubit] = IntegratedResults(I + 1j * Q)


This is again software demodulation.
It is slightly different from @stavros11 function:

qibolab/src/qibolab/instruments/qblox/acquisition.py

Lines 9 to 23 in 4f2e6b6

def demodulate(input_i, input_q, frequency):

"""Demodulates and integrates the acquired pulse."""

# DOWN Conversion

# qblox does not remove the offsets in hardware

modulated_i = input_i - np.mean(input_i)

modulated_q = input_q - np.mean(input_q)

num_samples = modulated_i.shape[0]

time = np.arange(num_samples)

phase = 2 * np.pi * frequency * time / SAMPLING_RATE

cosalpha = np.cos(phase)

sinalpha = np.sin(phase)

demod_matrix = np.sqrt(2) * np.array([[cosalpha, sinalpha], [-sinalpha, cosalpha]])

result = np.einsum("ijt,jt->i", demod_matrix, np.stack([modulated_i, modulated_q]))

return np.sqrt(2) * result / num_samples

but I guess the main reason is that here you're using the whole frequency, while there it deals separately the IF and LO (thus he already receives the I and Q components, still partially modulated, and demodulate them with a matrix).
But one (yours) should be a particular case of the other, so we should be able to recast with a unique implementation.

In any case, I wanted to point out, to extract this part later on as well.

alecandido · 2024-01-11T17:21:18Z

src/qibolab/instruments/icarusqfpga.py

+            if options.averaging_mode is not AveragingMode.SINGLESHOT:
+                results[readout_pulse.qubit] = results[readout_pulse.serial] = results[
+                    readout_pulse.qubit
+                ].average
+            else:
+                results[readout_pulse.serial] = results[readout_pulse.qubit]


Why are you actually doubling the results entries?

I understand that you need to decide to average or not, but this you could decide above:

singleshot = IntegratedResults(I + 1j * Q) results[readout_pulse.qubit] = singleshot.average if options.averaging_mode is not AveragingMode.SINGLESHOT else singleshot

Why are you registering for both readout_pulse.qubit and readout_pulse.serial?

This is largely due to old code standard before we used readout_pulse.serial. I have defaulted to serial being the default key. However, due to the mutable sweepers, I still need to keep qubit as the key.

This is a temporary workaround and will be removed in a future PR.

Ok, agreed.

In the meanwhile, I'd just open an issue for this, and close this comment. If you wish you could do it, otherwise I can.

alecandido · 2024-01-11T17:22:44Z

src/qibolab/instruments/icarusqfpga.py

        """
+        super().play(qubits, couplers, sequence, options)
+        self.device.set_adc_trigger_repetition_rate(int(options.relaxation_time / 1e3))


Further conversion

src/qibolab/instruments/icarusqfpga.py

alecandido · 2024-01-11T17:45:39Z

src/qibolab/instruments/icarusqfpga.py

+        res = self._sweep(qubits, couplers, sequence, options, *sweeper)
+
+        # Reset pulse values back to original values
+        for sweep, base_sweeper_values in zip(sweeper, bsv):
+            param_name = sweep.parameter.name.lower()
+            for pulse, value in zip(sweep.pulses, base_sweeper_values):
+                setattr(pulse, param_name, value)


Instead of resetting, I'd rather avoid modifying the original sequence inside _sweep().
Just make a copy before.

(even though I admit that it might be less trivial than expected, since the sweeper holds a reference to the pulse, and no other way to address it...)

Yeah, I will eventually move this to hardware, so this is just a temporary workaround.

I know the software sweeper is not here to stay. I just pointed it out anyhow, because you never know how long temporary workarounds will actually last.

However, I won't stop the merge of this PR just because of it. So, if you don't want to commit time on this, feel free to ignore.

alecandido · 2024-01-11T17:47:00Z

src/qibolab/instruments/icarusqfpga.py

+                if pulse.type is PulseType.READOUT:
+                    res[pulse.serial] = res[pulse.qubit]


Here you are registering also for serials, but only for the READOUT pulses swept. Is it correct? Why should it be different for READOUT pulses that are swept or not?

EDIT: in particular, it should have already been done by .play() (that is the one called at the bottom of the recursion)

The current representation of the readout pulse serial is

return f"ReadoutPulse({self.start}, {self.duration}, {format(self.amplitude, '.6f').rstrip('0').rstrip('.')}, {format(self.frequency, '_')}, {format(self.relative_phase, '.6f').rstrip('0').rstrip('.')}, {self.shape}, {self.channel}, {self.qubit})"

For the mutable temporary sweepers I have at the moment, as the readout pulse parameter is being modified, so too is the serial in every iteration. This makes the final result tied to an accumulation of different readout pulses rather than the original pulse serial.

Its not nice to have the multiple keys, but its a temp workaround for qibocal compatiabilty. This will be removed once I move the sweepers to hardware.

Ok, thanks for the explanation.

src/qibolab/instruments/icarusqfpga.py

sorewachigauyo · 2024-01-12T03:46:19Z

Our fridge is currently cooling down, so I will test the code sometime on Monday

…sq-dev

sorewachigauyo · 2024-01-18T03:43:37Z

In terms of functionality, the execute_pulse_sequence and sweep methods works, so we should be good to go.
Qibocal isn't working because qiboteam/qibocal#688 is still pending with the new platform changes.

I'll do further tests with the qubits without qibocal.

sorewachigauyo · 2024-01-18T06:08:20Z

Works with the qubits, so should be good to go
@alecandido @stavros11

alecandido

There are a few comments left from the previous review, some of them pretty simple to apply.

In any case, ignore those related to conversion, we need a consistent scheme also in the rest of Qibolab, so whatever you did it's just fine.

alecandido · 2024-01-18T07:30:28Z

src/qibolab/instruments/icarusqfpga.py

+                end = start + len(i_env)
+                t = np.arange(start, end) / dac_sampling_rate
+                cosalpha = np.cos(
+                    2 * np.pi * pulse.frequency * t + pulse.relative_phase
+                )
+                sinalpha = np.sin(
+                    2 * np.pi * pulse.frequency * t + pulse.relative_phase
+                )
+                wfm = i_env * sinalpha + q_env * cosalpha


The final comment (split a function, but keep in this module) would save you some repetition.

stavros11

Thanks for the updates. I wrote some comments below.

Additionally, it would be good to have this included in (non-QPU) tests. At this stage not explicit tests (test_instruments_icarusq), as these are not very useful anyway, mostly a platform (create + runcard), similar to the ones in tests/dummy_qrc. This also serves as a template/guide on how a platform containing these instruments looks.

If it works as it is, I don't think any of these comments are blocking for the merge, except maybe the one about the sampling rate to property (which is easy to fix anyway). However I would consider the issues of setup and connection particularly in case we need to rethink our approach to make it more usable.

src/qibolab/instruments/icarusqfpga.py

stavros11 · 2024-01-19T11:52:11Z

src/qibolab/instruments/icarusq.py

+    @attenuation.setter
+    def attenuation(self, attenuation: float):
        http = urllib3.PoolManager()
        http.request("GET", f"http://{self.address}/SETATT={attenuation}")


This is directly uploading the attenuation value to the instrument and similarly the getter grabs it for the instrument. This is opposite approach to what is used in the other instruments where we keep a cache of parameters even before connecting. Not a big issue as this instrument is quite simple, but it could cause complication if you want to set (eg. load from runcard) values before connecting.

src/qibolab/instruments/icarusqfpga.py

stavros11 · 2024-01-19T11:54:38Z

src/qibolab/instruments/icarusqfpga.py

-        pass
-
-    def disconnect(self):
+    def setup(self):


setup is not implemented in any of these instruments, so it is not possible to load settings from the runcard YAML. This may be a bit annoying if you want to have a routine that calibrates some instrument parameter, but if no, it should be fine.

stavros11 · 2024-01-19T11:58:55Z

src/qibolab/instruments/icarusqfpga.py

+                end = start + len(i_env)
+                t = np.arange(start, end) / dac_sampling_rate
+                cosalpha = np.cos(
+                    2 * np.pi * pulse.frequency * t + pulse.relative_phase
+                )
+                sinalpha = np.sin(
+                    2 * np.pi * pulse.frequency * t + pulse.relative_phase
+                )
+                wfm = i_env * sinalpha + q_env * cosalpha


To expand on this, it could even be useful to lift these functions outside the drivers as they are not really instrument specific and there is a similar function on qblox. I believe this has been discussed before, maybe we should convert to an issue (if there isn't one already).

stavros11

Thanks @sorewachigauyo. If this is working for you, feel free to merge.

sorewachigauyo added 9 commits June 12, 2023 18:02

icarusq instrument base

a1cd446

driver update for icarusq alpha 21

ee23a2d

IcarusQ alpha21.1 support

987784d

Merge branch 'main' of https://github.com/qiboteam/qibolab into icaru…

5108d97

…sq-dev

single sweeper implementation

e54c675

Merge branch 'main' of https://github.com/qiboteam/qibolab into icaru…

86d2537

…sq-dev

Update icarusq drivers

6726066

icarusq driver pylint

90ac98a

icarusq driver pylint fix

f1f390a

scarrazza requested review from GabrielePalazzo and PiergiorgioButtarini November 29, 2023 08:51

scarrazza added this to the Qibolab 0.1.4 milestone Nov 29, 2023

alecandido reviewed Nov 30, 2023

View reviewed changes

src/qibolab/instruments/icarusq.py Outdated Show resolved Hide resolved

src/qibolab/instruments/icarusqfpga.py Outdated Show resolved Hide resolved

src/qibolab/instruments/icarusqfpga.py Outdated Show resolved Hide resolved

src/qibolab/instruments/icarusqfpga.py Show resolved Hide resolved

stavros11 modified the milestones: Qibolab 0.1.4, Qibolab 0.1.5 Dec 6, 2023

sorewachigauyo added 5 commits December 7, 2023 10:44

Merge branch 'main' of https://github.com/qiboteam/qibolab into icaru…

ee8d4a7

…sq-dev

IcarusQ sweeper implementation and import cleanup

c366a82

Merge branch 'main' of https://github.com/qiboteam/qibolab into icaru…

1b1dcbb

…sq-dev

Use qibolab pulse envelopes and add deps

ed58ea6

IcarusQ update for Qibocal and sweepers

4f2e6b6

alecandido self-requested a review January 10, 2024 19:09

alecandido reviewed Jan 11, 2024

View reviewed changes

sorewachigauyo added 2 commits January 12, 2024 11:03

Update icarusq instrument methods

c08af5d

Update icarusq rfsoc readout and sweeper

b87ea2c

alecandido mentioned this pull request Jan 12, 2024

Pulse simplification #683

Closed

19 tasks

sorewachigauyo added 2 commits January 18, 2024 10:49

Merge branch 'main' of https://github.com/qiboteam/qibolab into icaru…

2581f9b

…sq-dev

Add sampling rate for IcarusQ

bb0f7a1

alecandido approved these changes Jan 18, 2024

View reviewed changes

stavros11 reviewed Jan 19, 2024

View reviewed changes

stavros11 mentioned this pull request Jan 22, 2024

Rename Qblox module classes #766

Merged

4 tasks

sorewachigauyo added 3 commits January 24, 2024 17:29

Add sampling rate property to IcarusQ RFSoC

a449052

Rework IcarusQ connect to be in line with other instruments

497e5a7

Fix change for pylint

4a0be6f

stavros11 approved these changes Jan 24, 2024

View reviewed changes

sorewachigauyo merged commit 2f6ae58 into main Jan 24, 2024
20 of 21 checks passed

stavros11 deleted the icarusq-dev branch January 24, 2024 10:18

		class RFSOC_RO(RFSOC):
		"""IcarusQ RFSoC attached with readout capability."""

	def demodulate(input_i, input_q, frequency):
	"""Demodulates and integrates the acquired pulse."""
	# DOWN Conversion
	# qblox does not remove the offsets in hardware
	modulated_i = input_i - np.mean(input_i)
	modulated_q = input_q - np.mean(input_q)

	num_samples = modulated_i.shape[0]
	time = np.arange(num_samples)
	phase = 2 * np.pi * frequency * time / SAMPLING_RATE
	cosalpha = np.cos(phase)
	sinalpha = np.sin(phase)
	demod_matrix = np.sqrt(2) * np.array([[cosalpha, sinalpha], [-sinalpha, cosalpha]])
	result = np.einsum("ijt,jt->i", demod_matrix, np.stack([modulated_i, modulated_q]))
	return np.sqrt(2) * result / num_samples

		if pulse.type is PulseType.READOUT:
		res[pulse.serial] = res[pulse.qubit]

Refactor IcarusQ drivers #661

Refactor IcarusQ drivers #661

Conversation

sorewachigauyo commented Nov 15, 2023

codecov bot commented Nov 15, 2023 • edited Loading

Codecov Report

scarrazza commented Nov 15, 2023

sorewachigauyo commented Nov 16, 2023

stavros11 commented Nov 16, 2023

sorewachigauyo commented Nov 17, 2023

stavros11 commented Nov 17, 2023

alecandido commented Jan 11, 2024

alecandido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stavros11 Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorewachigauyo commented Jan 12, 2024

sorewachigauyo commented Jan 18, 2024

sorewachigauyo commented Jan 18, 2024

alecandido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stavros11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stavros11 left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 15, 2023 •

edited

Loading

stavros11 Jan 12, 2024 •

edited

Loading