Patch RFSoC result shape #914

rodolfocarobene · 2024-06-10T12:57:54Z

Closes #913. The patch is quite ugly, moreover I am not sure if it works for every case... I've tested it only for a single qubit sweep (readout frequency).

I guess at some point it was introduced that the results must be squeezed like for dummy:

qibolab/src/qibolab/instruments/dummy.py

Lines 130 to 134 in 2e6c082

    
           for ro_pulse in sequence.ro_pulses: 
        
               values = np.squeeze(self.get_values(options, ro_pulse, shape)) 
        
               results[ro_pulse.qubit] = results[ro_pulse.serial] = options.results_type( 
        
                   values 
        
               )

I was instead always returning values in the form of [[val1, val2, val3, val4...]] (not sure why, maybe it was wrong from the beginning). However, downgrading qibolab at my last commit and qibocal at 0.0.8 "solves" the issue, so maybe something was introduced in the middle? Do you, @stavros11, @alecandido , @hay-k, recall something specific?

Moreover, are there plans to introduce again some automatic tests with instruments? I wanted to add a specific test that could have caught this problem before

Checklist:

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

codecov · 2024-06-10T12:58:22Z

Codecov Report

Attention: Patch coverage is 88.88889% with 6 lines in your changes missing coverage. Please review.

Project coverage is 66.97%. Comparing base (2e6c082) to head (56b0b02).
Report is 162 commits behind head on main.

Files	Patch %	Lines
src/qibolab/instruments/rfsoc/driver.py	88.23%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #914      +/-   ##
==========================================
+ Coverage   66.90%   66.97%   +0.06%     
==========================================
  Files          58       58              
  Lines        5983     6001      +18     
==========================================
+ Hits         4003     4019      +16     
- Misses       1980     1982       +2

Flag	Coverage Δ
unittests	`66.97% <88.88%> (+0.06%)`	⬆️

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.

stavros11 · 2024-06-10T13:57:54Z

I was instead always returning values in the form of [[val1, val2, val3, val4...]] (not sure why, maybe it was wrong from the beginning). However, downgrading qibolab at my last commit and qibocal at 0.0.8 "solves" the issue, so maybe something was introduced in the middle? Do you, @stavros11, @alecandido , @hay-k, recall something specific?

There were several issues with the shapes returned by different drivers in the past, and several attempts to fix them, so I do not recall which could be the one that broke rfsoc. I am also not sure what is the status right now, other than that dummy and QM were working, at least as qibocal expects, with the latest main. I believe we have also fixed ZI accordingly, but maybe @andrea-pasquale has more info on other instruments.

Moreover, are there plans to introduce again some automatic tests with instruments? I wanted to add a specific test that could have caught this problem before

I think the tests in https://github.com/qiboteam/qibolab/blob/main/tests/test_result_shapes.py were introduced to exactly check the shapes and I remember having them passing with different instruments, but probably we never tried rfsoc. Do these tests work for you? If there is a case that is not captured by them maybe we need to extend them.

You cannot run them in GitHub CI, but if you have access to a platform, you can still run manually as

pytest -m qpu --no-cov --platform {platform_name} tests/test_result_shapes.py

alecandido · 2024-06-11T04:03:07Z

@stavros11 is right, and recently there has been some rework in ZI for that. I inspected some of these issues after qiboteam/qibocal#737, and I reported in there (e.g. qiboteam/qibocal#737 (comment)).

Summary: this was triggered by the single shot acquisition in order to implement standard deviations' calculation, that added one more dimension to many routines.

In principle, the results should just be an array, with a shape of the kind (iq/raw/..., shots, sweeper1, sweeper2, ...), where sweepers are sorted according to the user sorting in the execution call.
However, the order was messed up in Zurich, because you necessarily need to sort differently to execute real-time sweepers inside near-time(/laptop-time) sweepers, and then rearranged before returning the result. @hay-k should have fixed it by now, but the problem was detected only now because the combination of single-shot + a specific/a certain combination of sweepers was never used before in routines.

We will to reassess anyhow in 0.2, because this will have an interplay with the diagonal(/simultaneous/...) sweepers. Though, if everything is already properly implemented, it should be a smooth extension.
In any case, in 0.2 the proposal is to simplify the results objects as well, also following #861, so we'll have to reassess anyhow :)

rodolfocarobene · 2024-06-11T11:20:53Z

Ok thanks @stavros11 and @alecandido! I will run tests/test_result_shapes.py as soon as I can. Depending on what the results will be I will extend the tests or/and ask you to merge #914

alecandido · 2024-06-19T05:48:41Z

src/qibolab/instruments/rfsoc/driver.py

+                        for pulse_idx in range(kdx + 1, len(sequence)):
+                            sequence[pulse_idx].start = sequence[pulse_idx - 1].duration


This is slightly better than the former patch, because it is less hard-coded for the Rabi.
However, it is still a quick and dirty patch, because it would advance even pulses that should be played in parallel.
E.g.:

drive (start=0, duration swept) flux (start=0) readout (start=drive.duration)

not only the readout pulse is advanced, but even the flux.

The proper fix for this is one of these:

diagonal (simultaneous) sweeper, and sweep not only the duration of the drive, but even the measurement start

add a delay on the readout line, and you can sweep the duration of the drive and readout delay in the same sweeper (this should be fixed in 0.2, if we'll have simultaneous sweepers, better to use one pulse per sweeper @stavros11)

add an align() between the drive and readout pulse

Since the diagonal sweepers and aligns are not yet supported by Qibolab, the only viable solution is 2., assuming you could mock a readout delay (like adding a readout pulse with 0 amplitude, and then discarding the result), but it would require to change the Qibocal routine.

To be fair, the Qibocal routine should be changed in any case, since the sweeper is just on the duration, and it is never specified that you should advance the reaodut pulse as well, so it might be up to the drivers to push measurements at the end, and it would be wrong (because in general, you should be free of playing the readout wherever you wish).

To be fair, the Qibocal routine should be changed in any case, since the sweeper is just on the duration, and it is never specified that you should advance the reaodut pulse as well, so it might be up to the drivers to push measurements at the end, and it would be wrong (because in general, you should be free of playing the readout wherever you wish).

@andrea-pasquale is it correct? In case, I'll open an issue on Qibocal

Yes, I believe that this is the case for the majority of protocols involving duration sweepers. Right now qibocal is leveraging the start attribute to hint at the fact that the readout should be pushed.
https://github.com/qiboteam/qibocal/blob/e66294c038aa42409160eae13fe7aca5641cde63/src/qibocal/protocols/rabi/utils.py#L265-L267
I know that in qibolab 0.2 start will be dropped, so in any case all this business related to the readout pulses being pushed when a previous pulse is swept will be addressed.

Right now qibocal is leveraging the start attribute to hint at the fact that the readout should be pushed.

The problem is that is not truly a hint, since the official specification of that would be that is exactly starting at that time (the .finish of the previous pulse) that is a number even in the case in which the other pulse is swept.
Pulses swept are normal pulses in the sequence, then the sweeper will later change its attributes, but there is no link with other pulses with accidentally the same values (or related ones) for same or other properties (like pulse1.finish == pulse2.start is a relation for the original sequence, but it is not checked in any way when the sweeper will modify the value of pulse1.duration, and thus its end).

alecandido · 2024-06-19T08:17:43Z

betterify the fix
4222de1

Improve? xD

src/qibolab/instruments/rfsoc/convert.py

rodolfocarobene · 2024-06-19T15:03:29Z

Ok thanks @stavros11 and @alecandido! I will run tests/test_result_shapes.py as soon as I can. Depending on what the results will be I will extend the tests or/and ask you to merge #914

All these tests now pass, with the exception of the ones involving "statistical_frequency" that I'm not sure how should behave

alecandido · 2024-06-19T16:58:44Z

All these tests now pass, with the exception of the ones involving "statistical_frequency" that I'm not sure how should behave

It appeared about one year ago in a merge commit f43c401 by @Jacfomg. But maybe is just a merge mess, and it's actually coming from someone else...

Do you remember what's its meaning?

(the only other person who may remember is @stavros11)

If no one knows, we could even schedule for removal...

Jacfomg · 2024-06-20T08:21:49Z

I believe it was me, but the naming convention was someone else. It is supposed to be for averaged results and it should be the just a number that it is the average of the shots or list of numbers in the case of a sweep made also from the average of the shots.

alecandido · 2024-06-20T08:26:27Z

But then, do you agree it could be a property? I.e. it can always be recomputed from the .samples attribute.

rodolfocarobene · 2024-06-20T08:57:16Z

But then, do you agree it could be a property? I.e. it can always be recomputed from the .samples attribute.

This seems to me a bit more reasonable that what we have now

alecandido · 2024-06-20T09:38:40Z

This seems to me a bit more reasonable that what we have now

If that's the case, I can make a PR in Qibolab 0.1 to replace it (it should be fairly easy to replace in all drivers).

stavros11 · 2024-06-20T10:49:30Z

But then, do you agree it could be a property? I.e. it can always be recomputed from the .samples attribute.

Is this about the statistical_frequency? Looking at the related test:

qibolab/tests/test_result_shapes.py

Line 61 in f335e4d

def test_discrimination_cyclic(connected_platform, sweep):

this is used when using (AcquisitionType.DISCRIMINATION, AveragingMode.CYCLIC) and if I interpret this correctly, in that case you will never get back all the samples from the instrument, but only their average value. Therefore, I am not sure if you can make it a property and recompute it from .samples. If that's correct, then I am not sure why AveragedSampleResults also accepts a samples argument.

As for the name, I don't know where it came from, but I believe probability would be more accurate.

alecandido · 2024-06-20T10:58:03Z

this is used when using (AcquisitionType.DISCRIMINATION, AveragingMode.CYCLIC) and if I interpret this correctly, in that case you will never get back all the samples from the instrument, but only their average value. Therefore, I am not sure if you can make it a property and recompute it from .samples. If that's correct, then I am not sure why AveragedSampleResults also accepts a samples argument.

Indeed, the point is that you should require only one between samples and statistical_frequency. In case, the other one is possible to be computed, it should be a property.

As for the name, I don't know where it came from, but I believe probability would be more accurate.

Fine with me for the new name.

Also note that in Qibocal this attribute is never used:
https://github.com/search?q=repo%3Aqiboteam%2Fqibocal+statistical_frequency&type=code
and in Qibolab is used in a single place, other than tests, i.e.

qibolab/src/qibolab/result.py

Lines 185 to 189 in f335e4d

    
               @lru_cache 
        
               def probability(self, state=0): 
        
                   """Returns the statistical frequency of the specified state (0 or 
        
                   1).""" 
        
                   return abs(1 - state - self.statistical_frequency)

(so statistical_frequency is essentially the probability of the 1 state)

stavros11 · 2024-06-20T11:05:35Z

Indeed, the point is that you should require only one between samples and statistical_frequency. In case, the other one is possible to be computed, it should be a property.

This property already exists here:

qibolab/src/qibolab/result.py

Line 151 in f335e4d

def average(self):

and calculates the AveragedSampleResults (hence the statistical_frequency/probability) from the samples.

I think with the current design of results, the only inconsistency is that AveragedSampleResults accepts samples. If samples are available from the instrument, then a SampleResults object should be created, not the averaged one.

src/qibolab/instruments/rfsoc/driver.py

alecandido · 2024-06-22T10:23:29Z

src/qibolab/instruments/rfsoc/driver.py

        for serial in dict_b:
            if serial in dict_a:
-                cls = dict_a[serial].__class__
-                if isinstance(dict_a[serial], IntegratedResults):
-                    new_data = np.column_stack(
-                        [dict_a[serial].voltage, dict_b[serial].voltage]
-                    )
-                elif isinstance(dict_a[serial], SampleResults):
-                    new_data = np.append(dict_a[serial].samples, dict_b[serial].samples)
-                dict_a[serial] = cls(new_data)
+                data = lambda res: (
+                    res.voltage if isinstance(res, IntegratedResults) else res.samples
+                )
+                dict_a[serial] = type(dict_a[serial])(
+                    np.append(data(dict_a[serial]), data(dict_b[serial]))
+                )
            else:
                dict_a[serial] = dict_b[serial]
        return dict_a


Not this PR business, but all this function would be just an np.append (or np.stack, along the out-enough dimension) if the results' objects were simple NumPy arrays...

(possibly even just an np.append, and later reshape/transpose...)

src/qibolab/instruments/rfsoc/driver.py

rodolfocarobene · 2024-06-22T12:39:06Z

The code is still a bit messy, but the main problem here is the result objects...
When they will be updated there will be a lot of simplification

alecandido

Small comment about static methods, but I'm ok with the fix.

Thank you very much! @rodolfocarobene

alecandido · 2024-06-22T18:11:02Z

src/qibolab/instruments/rfsoc/driver.py

+    @staticmethod
+    def validate_input_command(
+        sequence: PulseSequence, execution_parameters: ExecutionParameters, sweep: bool
+    ):


Here and above, since these methods does not make of self (as all @staticmethods) you could even consider keeping them out of the class.

To me, the static methods are useful to bring a function together with a class, such that it is available whenever a class is available.
If, instead, you want to have a function to be used inside the class, then just define a separate function in the same module (and, if it is to be used only by that class, prepend an _ to its name :P).

rodolfocarobene · 2024-06-24T12:28:11Z

Thanks for the review @alecandido! Maybe after we finish the measures in our lab I can try to fix some coding style issues in another PR.
Would it be possible to have this reviewed soon? Because right now the RFSoC is completely not working on main

stavros11

Thanks @rodolfocarobene. I am not sure if we have any platform available to test this here, so I just trust it works.

np.squeeze final result in sweeps

f348b94

rodolfocarobene added 4 commits June 18, 2024 15:38

fix shape

a0e3d40

partial fix

be86c86

partial fix for duration sweepers

ff5f919

fix shape of averaged discriminated

0a5b284

alecandido reviewed Jun 19, 2024

View reviewed changes

alecandido mentioned this pull request Jun 19, 2024

Timing and sweepers qiboteam/qibocal#891

Open

betterify the fix

4222de1

fix amplitude sweepers

fc3963e

alecandido reviewed Jun 19, 2024

View reviewed changes

src/qibolab/instruments/rfsoc/convert.py Outdated Show resolved Hide resolved

review

26f57c4

rodolfocarobene marked this pull request as ready for review June 21, 2024 10:18

rodolfocarobene requested review from alecandido, andrea-pasquale and stavros11 June 21, 2024 10:18

alecandido reviewed Jun 22, 2024

View reviewed changes

review

56b0b02

alecandido approved these changes Jun 22, 2024

View reviewed changes

stavros11 approved these changes Jun 24, 2024

View reviewed changes

rodolfocarobene merged commit c832cb0 into main Jun 24, 2024
24 checks passed

scarrazza deleted the patch_rfsoc_res_shape branch June 26, 2024 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patch RFSoC result shape #914

Patch RFSoC result shape #914

rodolfocarobene commented Jun 10, 2024

codecov bot commented Jun 10, 2024 •

edited

Loading

stavros11 commented Jun 10, 2024

alecandido commented Jun 11, 2024

rodolfocarobene commented Jun 11, 2024

alecandido Jun 19, 2024

alecandido Jun 19, 2024

andrea-pasquale Jun 19, 2024

alecandido Jun 19, 2024

alecandido commented Jun 19, 2024 •

edited

Loading

rodolfocarobene commented Jun 19, 2024

alecandido commented Jun 19, 2024 •

edited

Loading

Jacfomg commented Jun 20, 2024

alecandido commented Jun 20, 2024

rodolfocarobene commented Jun 20, 2024

alecandido commented Jun 20, 2024

stavros11 commented Jun 20, 2024

alecandido commented Jun 20, 2024 •

edited

Loading

stavros11 commented Jun 20, 2024

alecandido Jun 22, 2024

rodolfocarobene commented Jun 22, 2024

alecandido left a comment

alecandido Jun 22, 2024

rodolfocarobene commented Jun 24, 2024

stavros11 left a comment

	for ro_pulse in sequence.ro_pulses:
	values = np.squeeze(self.get_values(options, ro_pulse, shape))
	results[ro_pulse.qubit] = results[ro_pulse.serial] = options.results_type(
	values
	)

		for pulse_idx in range(kdx + 1, len(sequence)):
		sequence[pulse_idx].start = sequence[pulse_idx - 1].duration

Patch RFSoC result shape #914

Patch RFSoC result shape #914

Conversation

rodolfocarobene commented Jun 10, 2024

codecov bot commented Jun 10, 2024 • edited Loading

Codecov Report

stavros11 commented Jun 10, 2024

alecandido commented Jun 11, 2024

rodolfocarobene commented Jun 11, 2024

alecandido Jun 19, 2024

Choose a reason for hiding this comment

alecandido Jun 19, 2024

Choose a reason for hiding this comment

andrea-pasquale Jun 19, 2024

Choose a reason for hiding this comment

alecandido Jun 19, 2024

Choose a reason for hiding this comment

alecandido commented Jun 19, 2024 • edited Loading

rodolfocarobene commented Jun 19, 2024

alecandido commented Jun 19, 2024 • edited Loading

Jacfomg commented Jun 20, 2024

alecandido commented Jun 20, 2024

rodolfocarobene commented Jun 20, 2024

alecandido commented Jun 20, 2024

stavros11 commented Jun 20, 2024

alecandido commented Jun 20, 2024 • edited Loading

stavros11 commented Jun 20, 2024

alecandido Jun 22, 2024

Choose a reason for hiding this comment

rodolfocarobene commented Jun 22, 2024

alecandido left a comment

Choose a reason for hiding this comment

alecandido Jun 22, 2024

Choose a reason for hiding this comment

rodolfocarobene commented Jun 24, 2024

stavros11 left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 10, 2024 •

edited

Loading

alecandido commented Jun 19, 2024 •

edited

Loading

alecandido commented Jun 19, 2024 •

edited

Loading

alecandido commented Jun 20, 2024 •

edited

Loading