Include update methods in all protocols #526

andrea-pasquale · 2023-09-20T06:46:52Z

This PR aims at removing the platform.updatemethod in qibolab by delegating to qibocal the update of the platform.
This is possible since qibocal has access to all the primitives in qibolab to peform this operation.

@alecandido @stavros11 I think that this is more or less what we discussed yesterday.
I'm encapsulating each parameter update in a function in update.py for two reasons:

some routines update the same parameters

the update sometimes is not trivial see for example the update of the readout frequency

qibocal/src/qibocal/update.py

Lines 8 to 16 in 36eeb5d

    
           def update_readout_frequency(results: dict[QubitId, float], platform: Platform): 
        
               """Update readout frequency value in platform for each qubit in results.""" 
        
               for qubit, freq in results.items(): 
        
                   mz = platform.qubits[qubit].native_gates.MZ 
        
                   freq_hz = int(freq * GHZ_TO_HZ) 
        
                   mz.frequency = freq_hz 
        
                   if mz.if_frequency is not None: 
        
                       mz.if_frequency = freq_hz - platform.get_lo_readout_frequency(qubit) 
        
                   platform.qubits[qubit].readout_frequency = freq_hz

it is easier to write tests for each update_* function as I am doing here

For now I implemented the new proposal only for resonator spectroscopy.
If you agree with it I will propagate the change for all protocols in qibocal.

Checklist:

Reviewers confirm new code works as expected.
Tests are passing.
Coverage does not decrease.
Documentation is updated.
Compatibility with Qibo modules (Please edit this section if the current pull request is not compatible with the following branches).
- Qibo: master
- Qibolab: main
- Qibolab_platforms_qrc: main

stavros11

Thanks for implementing this @andrea-pasquale. It looks good to me, so I would say to go ahead with the rest of methods (after @alecandido's review).

The only thing that seems a bit annoying is that the for loop over qubits is repeated in all update methods. One way to solve this would be to lift it to Task.update_platform but I am not sure if this will create problems in the longer term, maybe for two-qubit gate routines, etc. where we may need full flexibility when defining _update.

src/qibocal/update.py

src/qibocal/auto/task.py

src/qibocal/update.py

alecandido · 2023-09-21T05:33:21Z

I agree with @stavros11, this is mostly fine, in that the overall structure is perfect (at least I'd not have any better proposal right now).
We might slightly improve the helper update functions (maybe), but I believe they should be in Qibocal. So, we'll always be in time to improve them.

Please, keep going. And thanks for this :)

codecov · 2023-09-22T05:07:04Z

Codecov Report

Merging #526 (6ef4b00) into main (4077637) will increase coverage by 0.12%.
Report is 10 commits behind head on main.
The diff coverage is 98.74%.

@@            Coverage Diff             @@
##             main     #526      +/-   ##
==========================================
+ Coverage   96.47%   96.59%   +0.12%     
==========================================
  Files          66       67       +1     
  Lines        4846     4998     +152     
==========================================
+ Hits         4675     4828     +153     
+ Misses        171      170       -1

Flag	Coverage Δ
unittests	`96.59% <98.74%> (+0.12%)`	⬆️

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

Files Changed	Coverage
src/qibocal/update.py	`95.94%`
src/qibocal/auto/execute.py	`100.00%`
src/qibocal/auto/operation.py	`100.00%`
src/qibocal/auto/task.py	`100.00%`
src/qibocal/fitting/classifier/run.py	`100.00%`
...tocols/characterization/allxy/drag_pulse_tuning.py	`100.00%`
...bocal/protocols/characterization/classification.py	`100.00%`
.../protocols/characterization/coherence/spin_echo.py	`100.00%`
...qibocal/protocols/characterization/coherence/t1.py	`100.00%`
...otocols/characterization/coherence/t1_sequences.py	`100.00%`
... and 20 more

andrea-pasquale · 2023-09-22T08:07:32Z

In principle this is done, I think.
I will see if I can find a way to repeat those for loops every time.
If I don't manage to do it, I will open an issue.

andrea-pasquale · 2023-09-22T08:09:53Z

src/qibocal/update.py

+    for qubit, beta in results.items():
+        shape = platform.qubits[qubit].native_gates.RX.shape
+        rel_sigma = re.findall(r"[\d]+[.\d]+|[\d]*[.][\d]+|[\d]+", shape)[0]
+        drag_pulse = pulses.Drag(rel_sigma=rel_sigma, beta=beta)
+        platform.qubits[qubit].native_gates.RX.shape = repr(drag_pulse)


@stavros11 I had to these weird shenanigans since apparently native_gates.RX.shape will give me the serial of the pulse shape and not the pulse shape object by itself.
Is there any reason to return the str and not the object itself?
I'm guessing that this is mostly related to the serialization of the native pulse

alecandido

Thanks @andrea-pasquale for this. To me, it's perfect (and it has been incredibly fast).

The problem of the loops (repeated operations for qubits and pairs) in a sense is a long-standing one, because it is related to the multi-qubit calibration, for which we never had a very clear plan.
The message is: if there is not already one, open a dedicated issue, and we will solve anyhow in a dedicated PR.

The only tiny remark is that you're mixing this relevant and general update with something specific to the Chevron, that maybe would have been worth to examine in a separate PR as well.

andrea-pasquale · 2023-09-22T08:51:08Z

The problem of the loops (repeated operations for qubits and pairs) in a sense is a long-standing one, because it is related to the multi-qubit calibration, for which we never had a very clear plan.
The message is: if there is not already one, open a dedicated issue, and we will solve anyhow in a dedicated PR.

Actually I might be close to find a solution for this.

The only tiny remark is that you're mixing this relevant and general update with something specific to the Chevron, that maybe would have been worth to examine in a separate PR as well.

Yeah, I know. I just did it since #520 is ready to merge I just need a second approval and I already fixed a few things there. I didn't want to open a second PR just for the chevron.

Finally, this PR does not tackle the update of the twpa parameters since the interface is currently being developed in qiboteam/qibolab#585

andrea-pasquale · 2023-09-22T10:24:10Z

Actually I might be close to find a solution for this.

For loop implemented :)

alecandido · 2023-09-22T15:27:50Z

For loop implemented :)

Saw it, that's definitely a possible solution.

I wonder if it's worth to have the whole routines working just for individual qubit/pair.
I believe the case of the fit will be similar to the update, but maybe it is convenient to make the acquisition in parallel (or maybe not, in which case we could really write the routines per qubit/pair, and keep the multi-thing at the level of the executor - in a separate PR, of course).

alecandido · 2023-09-22T15:28:59Z

src/qibocal/auto/task.py

@@ -94,6 +94,11 @@ def update(self):
        """Local update parameter."""
        return self.action.update

+    def update_platform(self, results: Results, platform: Platform):
+        if self.update:
+            for qubit in self.qubits:


self.qubits are actually pairs for two-qubits routines, isn't it?

(btw, maybe a brief docstring for this function would be nice)

alecandido · 2023-09-22T17:36:13Z

@andrea-pasquale since the Chevron might require slightly more time, would you consider removing it from here, such that we can immediately merge this in main?
If you wish, operationally:

make another branch update_chevron that duplicates this
edit this to remove the Chevron related part
merge this into main
merge update_chevron into chevron_fit
merge main into update_chevron (in principle, this step should do nothing)

Or is there any other limitation for which we need the Chevron to merge the update?

stavros11

Or is there any other limitation for which we need the Chevron to merge the update?

I think they are independent, so it should be possible to merge this before the Chevron fit. However, given that the Chevron fit is an enhancement and does not break any existing functionality, I am not sure if it is worth going through the trouble reversing the order now. The fit most likely won't be perfect but if it is on main it will be easier to test and improve.

The current PR also looks good to be merged.

I wonder if it's worth to have the whole routines working just for individual qubit/pair. I believe the case of the fit will be similar to the update, but maybe it is convenient to make the acquisition in parallel (or maybe not, in which case we could really write the routines per qubit/pair, and keep the multi-thing at the level of the executor - in a separate PR, of course).

For fit it may be possible, however for the acquisition it may be complex. With the current qibolab, if you want to take advantage of multiplex, you need to create a PulseSequence that targets multiple qubits and send it in a single platform.execute_pulse_sequence call. Lifting that to the executor will reduce our flexibility when writing routines, for example what if you want to execute two different sequences, on multiple qubits each. There may be a better design that allows this though.

stavros11 · 2023-09-22T19:00:25Z

src/qibocal/update.py

+    shape = platform.qubits[qubit].native_gates.RX.shape
+    rel_sigma = re.findall(r"[\d]+[.\d]+|[\d]*[.][\d]+|[\d]+", shape)[0]


I have not tested but this can probably be simplified using

Suggested change

shape = platform.qubits[qubit].native_gates.RX.shape

rel_sigma = re.findall(r"[\d]+[.\d]+|[\d]*[.][\d]+|[\d]+", shape)[0]

pulse = platform.qubits[qubit].native_gates.RX.pulse(start=0)

rel_sigma = pulse.shape.rel_sigma

This will basically delegate the shenanigans to qibolab's pulse constructor, but better to have them in a single place instead of two.

Thanks @stavros11, I'll give it a try.

andrea-pasquale · 2023-09-25T06:36:10Z

Or is there any other limitation for which we need the Chevron to merge the update?

I think they are independent, so it should be possible to merge this before the Chevron fit. However, given that the Chevron fit is an enhancement and does not break any existing functionality, I am not sure if it is worth going through the trouble reversing the order now. The fit most likely won't be perfect but if it is on main it will be easier to test and improve.

In theory they are independent, I decided to make this PR point to chevron_fit only because I could already implement the update for the chevron given that I the fitting is working in that branch.

I wonder if it's worth to have the whole routines working just for individual qubit/pair. I believe the case of the fit will be similar to the update, but maybe it is convenient to make the acquisition in parallel (or maybe not, in which case we could really write the routines per qubit/pair, and keep the multi-thing at the level of the executor - in a separate PR, of course).

For fit it may be possible, however for the acquisition it may be complex. With the current qibolab, if you want to take advantage of multiplex, you need to create a PulseSequence that targets multiple qubits and send it in a single platform.execute_pulse_sequence call. Lifting that to the executor will reduce our flexibility when writing routines, for example what if you want to execute two different sequences, on multiple qubits each. There may be a better design that allows this though.

I agree with pretty much everything that @stavros11 said.
The main problem here is multiplexing. A possible way to solve it would be to have another function before acquisition something like setup which should prepare the pulse sequence so that in the acquisition we merge and execute multiple pulse sequences...
I would say that this is out of scope of this PR, but we should definitely think about it.

alecandido · 2023-09-25T07:30:40Z

I would say that this is out of scope of this PR, but we should definitely think about it.

I agree it is out of scope for the PR, but it is not even urgent right now in general. We can rediscuss when we'll have lifted the main bottlenecks for autocalibration (and this is not one of them).

alecandido · 2023-09-25T10:56:40Z

I'd like this to be merged asap, in order to:

remove update from Qibolab
avoid merging another update extension in David/qubit tracking qibolab#581
avoid breaking it right after merging

So, if you want to merge #520 before, that's even fine. But if possible, let's do it soon.

Otherwise, the proposal above is because I do not feel much pressure about merging #520, but I'd like to merge this soon.
If you wish, I could follow the procedure above myself, and you can provide feedback.

andrea-pasquale · 2023-09-25T10:59:31Z

I'd like this to be merged asap, in order to:

remove update from Qibolab

avoid merging another update extension in David/qubit tracking qibolab#581

avoid breaking it right after merging

So, if you want to merge #520 before, that's even fine. But if possible, let's do it soon.

Otherwise, the proposal above is because I do not feel much pressure about merging #520, but I'd like to merge this soon. If you wish, I could follow the procedure above myself, and you can provide feedback.

I'm about to push a few fixes here but in principle both this PR and #520 are ready to merge. I prefer to merge #520 first I don't think that it is necessary to merge this one and afterwards #520.

andrea-pasquale · 2023-09-25T11:02:50Z

As soon as CI passes I will merge also this one.

alecandido · 2023-09-25T11:04:44Z

As soon as CI passes I will merge also this one.

Thank you very much :D

andrea-pasquale added 2 commits September 20, 2023 10:38

Refactor update for resonator spectroscopy

36eeb5d

Add tests

56607c7

andrea-pasquale requested review from stavros11 and alecandido September 20, 2023 06:46

stavros11 reviewed Sep 20, 2023

View reviewed changes

src/qibocal/update.py Outdated Show resolved Hide resolved

alecandido reviewed Sep 21, 2023

View reviewed changes

src/qibocal/auto/task.py Outdated Show resolved Hide resolved

src/qibocal/update.py Outdated Show resolved Hide resolved

src/qibocal/update.py Outdated Show resolved Hide resolved

This was referenced Sep 21, 2023

Add CZ and virtual Z phase to platform.update qiboteam/qibolab#582

Closed

David/qubit tracking qiboteam/qibolab#581

Merged

Stavros and Alessandro suggestions

e19e064

andrea-pasquale mentioned this pull request Sep 21, 2023

Standardize units of measure between fitting and runcard #530

Closed

andrea-pasquale added 3 commits September 21, 2023 17:16

Continue update implementation

6ddaae1

Merge branch 'chevron_fit' into refactor_update

115003b

Continue with other protocols

8c13d45

andrea-pasquale changed the base branch from main to chevron_fit September 22, 2023 04:32

andrea-pasquale changed the base branch from chevron_fit to main September 22, 2023 04:32

Fix tests by addind dummy update

f07f85e

andrea-pasquale self-assigned this Sep 22, 2023

andrea-pasquale added the enhancement New feature or request label Sep 22, 2023

andrea-pasquale added this to the Qibocal 0.0.4 milestone Sep 22, 2023

andrea-pasquale added 2 commits September 22, 2023 09:15

Improve coverage

84c9a3c

Finish implementing protocols update

51d1246

andrea-pasquale commented Sep 22, 2023

View reviewed changes

alecandido approved these changes Sep 22, 2023

View reviewed changes

Remove for loop

bc364b3

andrea-pasquale changed the base branch from main to chevron_fit September 22, 2023 10:25

Merge branch 'chevron_fit' into refactor_update

f690140

alecandido approved these changes Sep 22, 2023

View reviewed changes

stavros11 approved these changes Sep 22, 2023

View reviewed changes

Merge branch 'chevron_fit' into refactor_update

36e4af5

Docstrings and improve beta update

aacc5bd

Base automatically changed from chevron_fit to main September 25, 2023 11:02

andrea-pasquale mentioned this pull request Sep 25, 2023

Remove update method from platform qiboteam/qibolab#596

Merged

4 tasks

Fix tests

6ef4b00

andrea-pasquale added this pull request to the merge queue Sep 25, 2023

Merged via the queue into main with commit aef9c74 Sep 25, 2023
21 checks passed

andrea-pasquale deleted the refactor_update branch September 25, 2023 12:19

This was referenced Sep 26, 2023

David/qubit tracking #519

Merged

Ro amplitude #521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include update methods in all protocols #526

Include update methods in all protocols #526

andrea-pasquale commented Sep 20, 2023 •

edited

Loading

stavros11 left a comment

alecandido commented Sep 21, 2023

codecov bot commented Sep 22, 2023 •

edited

Loading

andrea-pasquale commented Sep 22, 2023

andrea-pasquale Sep 22, 2023

alecandido left a comment •

edited

Loading

andrea-pasquale commented Sep 22, 2023

andrea-pasquale commented Sep 22, 2023

alecandido commented Sep 22, 2023

alecandido Sep 22, 2023

alecandido commented Sep 22, 2023

stavros11 left a comment •

edited

Loading

stavros11 Sep 22, 2023

andrea-pasquale Sep 25, 2023

andrea-pasquale commented Sep 25, 2023

alecandido commented Sep 25, 2023

alecandido commented Sep 25, 2023

andrea-pasquale commented Sep 25, 2023

andrea-pasquale commented Sep 25, 2023

alecandido commented Sep 25, 2023

	def update_readout_frequency(results: dict[QubitId, float], platform: Platform):
	"""Update readout frequency value in platform for each qubit in results."""
	for qubit, freq in results.items():
	mz = platform.qubits[qubit].native_gates.MZ
	freq_hz = int(freq * GHZ_TO_HZ)
	mz.frequency = freq_hz
	if mz.if_frequency is not None:
	mz.if_frequency = freq_hz - platform.get_lo_readout_frequency(qubit)
	platform.qubits[qubit].readout_frequency = freq_hz

		shape = platform.qubits[qubit].native_gates.RX.shape
		rel_sigma = re.findall(r"[\d]+[.\d]+\|[\d]*[.][\d]+\|[\d]+", shape)[0]

Include update methods in all protocols #526

Include update methods in all protocols #526

Conversation

andrea-pasquale commented Sep 20, 2023 • edited Loading

stavros11 left a comment

Choose a reason for hiding this comment

alecandido commented Sep 21, 2023

codecov bot commented Sep 22, 2023 • edited Loading

Codecov Report

andrea-pasquale commented Sep 22, 2023

andrea-pasquale Sep 22, 2023

Choose a reason for hiding this comment

alecandido left a comment • edited Loading

Choose a reason for hiding this comment

andrea-pasquale commented Sep 22, 2023

andrea-pasquale commented Sep 22, 2023

alecandido commented Sep 22, 2023

alecandido Sep 22, 2023

Choose a reason for hiding this comment

alecandido commented Sep 22, 2023

stavros11 left a comment • edited Loading

Choose a reason for hiding this comment

stavros11 Sep 22, 2023

Choose a reason for hiding this comment

andrea-pasquale Sep 25, 2023

Choose a reason for hiding this comment

andrea-pasquale commented Sep 25, 2023

alecandido commented Sep 25, 2023

alecandido commented Sep 25, 2023

andrea-pasquale commented Sep 25, 2023

andrea-pasquale commented Sep 25, 2023

alecandido commented Sep 25, 2023

andrea-pasquale commented Sep 20, 2023 •

edited

Loading

codecov bot commented Sep 22, 2023 •

edited

Loading

alecandido left a comment •

edited

Loading

stavros11 left a comment •

edited

Loading