Adding calibration parameters to qibocal #1021

andrea-pasquale · 2024-10-22T07:59:20Z

This PR will restore the characterization section of the platform runcards that was removed during the update to 0.2 by introducing a calibration.json file.

Fix serialization of two qubit calibration container + add them to calibration runcard
Add readout mitigationa and crosstalk matrix
Decide where to put coupling
Add errors to certain parameters (?)

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: Restore calibration parameters qibolab_platforms_qrc#192

alecandido · 2024-10-22T08:36:11Z

Which is the reason why you eventually decided to base this on https://github.com/qiboteam/qibocal/tree/0.2? (just to be aware)

andrea-pasquale · 2024-10-22T08:41:42Z

Which is the reason why you eventually decided to base this on https://github.com/qiboteam/qibocal/tree/0.2? (just to be aware)

Given that this is required for 0.2 I thought that it could be more useful to start from the 0.2 branch. I know that before I said to you that I would have started from main :) Eventually I could rebase to main or another branch when necessary.

andrea-pasquale · 2024-10-25T17:40:14Z

I manage to restore the update for the parameters in calibration section for all protocols which have been ported to 0.2 in #990. As more protocols are added there I will update them also here.
This could be a good time to start discussing about the interface which is currently being implemented in this PR.
If there are important changes I suggest to them now, otherwise we can always fine tune the implementation thanks to the fact that now the calibration parameters are disentangled from qibolab.

src/qibocal/calibration/platform.py

alecandido · 2024-10-25T17:59:56Z

@andrea-pasquale actually I took a look, and to me, we could even merge this to #996, to avoid maintaining two branches, one on top of the other.

Of course, I'd wait even for the others to take a look as well, since it is a good chance to do this with the isolated diff (rather than mixed together with the rest of the 0.2 update).

andrea-pasquale · 2024-10-28T07:54:27Z

@stavros11 as soon as you approve these changes we can decide where to merge this PR. Currently it is pointing to #990 but we could even merge it to #996. Let me know how you prefer to proceed.

alecandido · 2024-10-28T08:42:15Z

I'd personally go for #990, as this is more final than #996 itself. But I'm open to different opinions

stavros11

Thanks @andrea-pasquale. I had a quick look and some comments, but I would also like to test something on hardware before merging. Have you tried to do so and check if the updates are dumped properly?

Other than that, my main comment is that this is reintroducing some of the repetition we had in qibolab 0.1, as some values appear in both the calibration and the parameters file. For example frequency_01 will be the same as the drive config frequency, sweetspot will be the same as the flux config offset, etc.. This is not necessarily bad, as it provides flexibility (eg. we may not want to operate the qubits at the sweetspot, etc.), however it will complicate the update process, both in the qibocal _update rules and the manual copy-pasting (if fitting fails) since we will again need to put the same value in multiple places. Personally, I don't have a strong opinion against, just mentioning it for discussion.

@stavros11 as soon as you approve these changes we can decide where to merge this PR. Currently it is pointing to #990 but we could even merge it to #996. Let me know how you prefer to proceed.

Another option, since this seems to be more advanced than #996, is to merge directly to #990 and I will rebase #996 on top of it. Note that #996 will change completely anyway to follow the idea of qiboteam/qibolab#1061, so I would avoid putting anything on top of it right now.

stavros11 · 2024-10-28T08:19:05Z

src/qibocal/calibration/calibration.py

+class Model(BaseModel):
+    """Global model, holding common configurations."""
+
+    model_config = ConfigDict(extra="forbid")


In qibolab the base Model is frozen. I guess here you are not doing this to make the updates easier, which makes sense to me.

Indeed, after discussing with @alecandido we decided to not have a frozen given that the update right now is quite cumbersome. If in qibolab we are able to find a suitable solution which is not too complicated we might go for a frozen model here as well.

src/qibocal/calibration/platform.py

stavros11 · 2024-10-28T08:29:28Z

src/qibocal/calibration/platform.py

+from pathlib import Path
+
+from qibolab import Platform, create_platform
+from qibolab._core.platform.load import PLATFORMS


Following up on this, I have not tested but it may actually be possible to get the path of a Platform with the current public API using something like:

from qibolab import locate_platform path = locate_platform(platform.name)

If this is sufficient, we won't need to release a new qibolab.

locate_platform seems to work correctly I didn't know about this function. Thanks!

stavros11 · 2024-10-28T08:31:45Z

src/qibocal/calibration/platform.py

+                (Path(__file__).parent / "dummy.json").read_text()
+            )
+        else:
+            path = Path(os.getenv(PLATFORMS)) / name


Looking at the functions in https://github.com/qiboteam/qibolab/blob/main/src/qibolab/_core/platform/load.py and in particuar: https://github.com/qiboteam/qibolab/blob/946a04f21f522cd05f898de91270153af7cdceff/src/qibolab/_core/platform/load.py#L25
I believe in qibolab we support storing multiple paths (with a separator) in the environment variable. I think this is not supported here, but again, I have not tested. This is also why it is safer to get the platform path using qibolab functions, instead of manually processing the environment variable here.

alecandido · 2024-10-28T09:07:32Z

Other than that, my main comment is that this is reintroducing some of the repetition we had in qibolab 0.1, as some values appear in both the calibration and the parameters file. For example frequency_01 will be the same as the drive config frequency, sweetspot will be the same as the flux config offset, etc.. This is not necessarily bad, as it provides flexibility (eg. we may not want to operate the qubits at the sweetspot, etc.), however it will complicate the update process, both in the qibocal _update rules and the manual copy-pasting (if fitting fails) since we will again need to put the same value in multiple places. Personally, I don't have a strong opinion against, just mentioning it for discussion.

Nice catch @stavros11!

Actually, I didn't investigate in depth the content of the calibration parameters, but just the structure of the implementation.
I'm a more opinionated than @stavros11: we should really avoid repetition.
Though my motivation is slightly different: for sure updating twice is more annoying, but what is worse is what happens when something goes wrong, and you do not update twice, but only in a single place, leading to an inconsistent configuration.
Let's rule out the room for inconsistencies as much as possible.

EDIT: I take it back: the parameters in Qibolab are the operational ones, but they do not necessarily correspond to the physical values of the system (e.g. you can decide to operate a qubit out of its sweetspot, for some reason, or to apply a certain detuning) - I don't see them as redundant any longer. What do you think @stavros11?

andrea-pasquale · 2024-10-28T09:58:11Z

Thanks @andrea-pasquale. I had a quick look and some comments, but I would also like to test something on hardware before merging. Have you tried to do so and check if the updates are dumped properly?

Thanks for the review @stavros11.
Yes, I already checked with qiboteam/qibolab_platforms_qrc#192 and the updates seems to work as expected.

Other than that, my main comment is that this is reintroducing some of the repetition we had in qibolab 0.1, as some values appear in both the calibration and the parameters file. For example frequency_01 will be the same as the drive config frequency, sweetspot will be the same as the flux config offset, etc.. This is not necessarily bad, as it provides flexibility (eg. we may not want to operate the qubits at the sweetspot, etc.), however it will complicate the update process, both in the qibocal _update rules and the manual copy-pasting (if fitting fails) since we will again need to put the same value in multiple places. Personally, I don't have a strong opinion against, just mentioning it for discussion.

As you said, I know that in theory we are repeating some parameters, but the idea is that you don't necessarily want to operate the qubit at the "calibration parameters". You already mentioned the sweetspot, but also for the drive frequency some papers are showing that you might get better performances by not driving the qubit at its 0->1 frequency but a slight detuning might help, especially when using DRAG. See for example Sect. IV C 3 here. The same goes for the readout frequency. I know that up to know we were talking about frequencies in a very generic way but I believe that if we are able to distinguish between the physical frequencies (resonator frequency or qubit frequency) and the frequencies at which we operate our devices this could be beneficial. Moreover, we can come up with protocols in qibocal that are targerting only some of those.

@stavros11 as soon as you approve these changes we can decide where to merge this PR. Currently it is pointing to #990 but we could even merge it to #996. Let me know how you prefer to proceed.

Another option, since this seems to be more advanced than #996, is to merge directly to #990 and I will rebase #996 on top of it. Note that #996 will change completely anyway to follow the idea of qiboteam/qibolab#1061, so I would avoid putting anything on top of it right now.

Sure, originally I pointed to 0.2 since with #996 I was getting some import errors.
Since this branch is already poiting to #990, it would be easier given that I don't have to rebase.

andrea-pasquale · 2024-10-28T10:13:34Z

src/qibocal/calibration/calibration.py

+from qibolab._core.identifier import QubitId, QubitPairId
+from qibolab._core.serialize import NdArray


Concerning the fact that it would be better to not import anything from _core also here I am importing a few things. I remember that there were some discussions in the past about exposing QubitId or QubitPairId. In this particular case QubitPairId is useful because I don't need to do anything to make sure that the pair is serialized correctly as a string. Eventually we could re-write some of these stuff in qibocal since we don't need to follow the exact serialization scheme of qibolab.

Thanks for pointing this out, I did not check in detail all changes.

I believe the initial idea for QubitId and QubitPairId was to not expose them from qibolab and therefore I had to redefine them in qibocal:

qibocal/src/qibocal/auto/operation.py

Lines 18 to 19 in d35ab6b

QubitId = Union[str, int]

QubitPairId = tuple[QubitId, QubitId]

After merging this we will be following two different approaches, the (private) import here and the redefinition everywhere else in qibocal, which is not optimal. If the deserialization of QubitPairId is really needed, we should consider if it's worth exposing it to public qibolab. I think qibocal will require a 0.2.1 anyway, in order to properly do parameter updates, so we could also include the QubitPairId there.

Yes, I agree with need a solution, though I'm not sure which is best. Maybe it's really correct to expose these identifiers, to avoid doubling the serialization support.

Instead, concerning releases:

most likely it will be 0.2.2, because we may have postponed everything by one release, to have immediately the OPX1000 released (since already being used)

@Edoardo-Pedicillo I may suggest renaming the milestone, since (in principle) we're not aiming to break Qibocal's interface (not that much, at least) and it will still be in the 0.1.x series (while introducing support for Qibolab 0.2)

you may use Qibocal - Qibolab 0.2 as temporary name to collect all these PRs, and then convert it to a milestone related to the actual targeted release in the 0.1.x series

stavros11

Thanks for the updates @andrea-pasquale. I also did a quick test and it seems to work, so it should be fine to merge in the 0.2 branch.

EDIT: I take it back: the parameters in Qibolab are the operational ones, but they do not necessarily correspond to the physical values of the system (e.g. you can decide to operate a qubit out of its sweetspot, for some reason, or to apply a certain detuning) - I don't see them as redundant any longer. What do you think @stavros11?

Indeed, these parameters are not strictly equivalent, we just happen to use the same values, that's why I said it's mostly for discussion.

However, in that case I would propose to go a step further and have different operations for updating each part. Qibocal's default _update (and qq update) should only update the calibration part and we provide a separate command or script that ports the changes to parameters (if the user wishes to do so). This new command will just copy some values from Calibration to Parameters (eg. frequency_01 will go to the drive configs, sweetspot to flux offsets, etc.) which is the default way we are operating right now.

Obviously this will add some overhead to usage (one more command), however I find it cleaner than the current approach, where we are duplicating the updates and essentially force using the qubits at their sweetspot.

EDIT: This suggestion is for another PR, not here. Actually #996 would be a good candidate to do this if you agree.

alecandido · 2024-10-28T13:39:18Z

@stavros11 what you say makes sense, but I wonder whether it is really a convenient approach.

In general, we may have some routines which are just characterizing the chip (finding things like frequency_01) and some other which may instead calibrate the operational parameters (to improve gates' fidelity, or other relevant metrics).
While the physical parameters will only populate the calibration part, the operational ones are those which are by definition stored in parameters.json. If we decide to always apply the two-steps approach, we may need to go beyond what is being done here, and double also the operational ones (e.g. the operational frequency), on top of those that are usually copied to them (e.g. the qubit frequency).

I.e., the two-steps approach requires:

parameters.json:
  RX.frequency: ...

calibration.json:
  RX.frequency: ...
  frequency_01: ...

otherwise it would be impossible to operate the qubit out of its frequency (or, conversely, to store its characterized frequency, if explicitly operated at a different one).

andrea-pasquale · 2024-10-28T13:51:27Z

Thanks for the proposal @stavros11.
I tend to agree with @alecandido on this one.
I think that it should be ok to have "double updates" at least at the beginning of the characterization process, where the operational frequencies are the same as the physical frequencies. Moreover, I would say that especially later on when we perform advanced operations such readout optimization we should only modify the readout frequency in the parameters.json.
To avoid confusion we could also try to classify the protocols in characterization protocols where we are going to modify both the parameters.json and the calibration.json and the calibration protocols where we are only going to modify the parameters.json file (if I finally understood correclty the difference between calibration and characterization).

Eventually we could add a command to update the parameters.json starting from the calibration.json, assuming that all operational frequencies will be set to the physical frequencies and the offset will be set to the sweetspot.

Edoardo-Pedicillo

Thanks @andrea-pasquale , I have left some comments. Could you please have a look to the tests ?

Edoardo-Pedicillo · 2024-10-28T14:34:27Z

src/qibocal/calibration/calibration.py

+    # TODO: Add something related to resonator calibration
+
+
+class Qubit(Model):


Here some parameters are missing like Ec, even if it is equal to the anharmonicity in first approximation we can try to evaluate it (so not having it as property returning the anharmonicity) and Ej.

Is it possible to measure the charging energy independently of the anharmonicity? (i.e. not measured as the frequencies' difference)

There is already a parameter for Ec, it is called charging_energy.
I could add also Ej I decided not to included immediately since we have no protocols which can compute Ej.
We can add some parameters later as soon as we have the corresponding protocols to evaluate them.

There is already a parameter for Ec, it is called charging_energy.

Correct, but my understanding was that @Edoardo-Pedicillo acknowledged that, but he was proposing to have it an actual attribute (instead of a property) such that it can have a value that is different from -anharmonicity.

And that's why I was asking whether it is possible to compute it independently.

Is it possible to measure the charging energy independently of the anharmonicity? (i.e. not measured as the frequencies' difference)

Yes, from the qubit flux dependecy protocol (https://qibo.science/qibocal/stable/protocols/flux/single.html), in theory also $E_J$ can be evaluated from there.

Ok, you're right. The qubit frequency (ground->excited) is already functionally dependent on $E_C$, though you need the whole flux-shape to fit it. And that's completely independent of the second transition frequency (they are only related through the $E_C$ parameter, but they are independent measurements).

Then I support your proposal: if we can measure both, we need two independent values, since they can be affected by approximations and the measurement process in different ways.

Then, if you only run a plain spectroscopy (identifying both transitions), you may compute the anharmonicity, but set also the charging energy as the best temporary estimate. Or avoid setting that at all, and do the fallback in the access process (if needed), to signal that the associated measurement has not been performed.

Is it possible to measure the charging energy independently of the anharmonicity? (i.e. not measured as the frequencies' difference)

Yes, from the qubit flux dependecy protocol (https://qibo.science/qibocal/stable/protocols/flux/single.html), in theory also E J can be evaluated from there.

Yes, but actually given that the fit is already quite complicated $E_C$ is not fitted there, but it is fixed.
Regarding $E_J$ I agree with you, I can compute it there as a function of the charging energy and the maximum frequency.

Edoardo-Pedicillo · 2024-10-28T14:37:20Z

src/qibocal/calibration/calibration.py

+        return (1 + self.fidelity) / 2
+
+
+class Coherence(Model):


Eventually I will take into consideration the error associated to this values. The same propagates for the other parameters.
I am not sure Coherence should be separated from Qubit, at the end these values are describing the qubit free evolution.

Sure, I can add errors for these parameters. I can also move them inside the Qubit class.

You could even define a type for:

Measure = tuple[float, float] """Measured central value and associated uncertainty."""

(that's just a rough sketch, pick the name and description you find more suitable)

Edoardo-Pedicillo · 2024-10-28T14:41:28Z

src/qibocal/calibration/calibration.py

+    """Readout information."""
+    coherence: Coherence = Field(default_factory=Coherence)
+    """Coherence times of the qubit."""
+    rb_fidelity: Optional[float] = None


We could have a Benchmarks class containing all the fidelities/metrics (eventually also the gate fidelities coming from the interleaved RB), and drop TwoQubitCalibration.

Maybe I misunderstood: in which way is it different from a renaming proposal?

Even if we create a Benchmark class containg all fidelities we will still need to have a TwoQubitCalibration object given that at least there should a class for each qubit or qubit pair.

Maybe I misunderstood: in which way is it different from a renaming proposal?

It was not proposing a renaming but a reordering, I was suggesting collecting all the fidelities together in a single class, since they are used for the same purpose, validation of the calibration.

Even if we create a Benchmark class containg all fidelities we will still need to have a TwoQubitCalibration object given that at least there should a class for each qubit or qubit pair.

Ok, I see your point, thanks.

src/qibocal/calibration/calibration.py

andrea-pasquale · 2024-10-28T18:39:38Z

Could you please have a look to the tests ?

There is not much that I can do about tests given that this PR is pointing at a branch where they are failing given that not all protocols have been updated to 0.2. I thought about adding some tests but given that these are just data structures they would be quite naive.

alecandido · 2024-10-28T20:06:53Z

There is not much that I can do about tests given that this PR is pointing at a branch where they are failing given that not all protocols have been updated to 0.2. I thought about adding some tests but given that these are just data structures they would be quite naive.

@andrea-pasquale you could test serialization and deserialization. E.g. making a closed loop, and checking the value is still consistent (that would be meaningful, because you would check for regressions in case of changes in some types).

stavros11 · 2024-10-29T14:51:57Z

There is not much that I can do about tests given that this PR is pointing at a branch where they are failing given that not all protocols have been updated to 0.2. I thought about adding some tests but given that these are just data structures they would be quite naive.

About this, there is also the option to skip tests of not updated routines in the 0.2 branch, however since pylint will also be failing due to the changes in qibolab interface (PulseSequence no longer has add, Platform no longer has sweep, etc.), the effort of fixing this is almost equivalent to fully updating the routines, so I would rather do the latter first.

Edoardo-Pedicillo

Thanks @andrea-pasquale ! LGTM

alecandido · 2024-11-07T08:43:06Z

Now that we arrived to 3 approvals (i.e. almost everyone saw the diff, and we have some consensus), shall we merge?

feat: Start implementation of calibration platform

e0bd457

feat: Add create_calibration_platform

7edd452

andrea-pasquale force-pushed the 0.2-cal branch from 0ff0fe8 to 7edd452 Compare October 22, 2024 11:19

andrea-pasquale added 5 commits October 22, 2024 16:21

chore: Working version on hardware

3151547

refactor: Calibration platform as inheritance of Platform

df45300

fix: dump platform correctly

dd620ba

refactor: Assignment fidelity as derived from ro fidelity

99533e6

feat: Update for classification and other protocols

25250ec

andrea-pasquale mentioned this pull request Oct 23, 2024

Restore calibration parameters qiboteam/qibolab_platforms_qrc#192

Merged

andrea-pasquale added 6 commits October 25, 2024 11:34

fix: Dump qubit pair as string

68313d4

build: Update lock file

879c3a1

docs: Minor adjustments

58efd52

refactor: Renaming + crosstalk matrix update

2cbfb87

refactor: resonator flux with new calibration model

2a07e1d

fix: Fixing #1018

1d7b990

andrea-pasquale mentioned this pull request Oct 25, 2024

Fitting for qubit spectroscopy fails on 0.2 branch #1018

Closed

andrea-pasquale added 2 commits October 25, 2024 21:28

feat: Restoring update in ramsey

1fced97

refactor: Restoring update for punchout

24a6ff2

alecandido reviewed Oct 25, 2024

View reviewed changes

src/qibocal/calibration/platform.py Outdated Show resolved Hide resolved

refactor: get PLATFORMS from qibolab

e844acf

alecandido approved these changes Oct 27, 2024

View reviewed changes

andrea-pasquale requested review from stavros11 and Edoardo-Pedicillo October 28, 2024 07:52

andrea-pasquale marked this pull request as ready for review October 28, 2024 07:54

stavros11 reviewed Oct 28, 2024

View reviewed changes

feat: Use locate_platform

124c01b

andrea-pasquale commented Oct 28, 2024

View reviewed changes

Edoardo-Pedicillo added this to the Qibocal 0.2.0 milestone Oct 28, 2024

stavros11 approved these changes Oct 28, 2024

View reviewed changes

Edoardo-Pedicillo requested changes Oct 28, 2024

View reviewed changes

andrea-pasquale added 3 commits October 29, 2024 13:53

feat: Add errors to coherence values

3ee5888

refactor: Convert measure to list to ease json serialization

6b4ec48

test: Adding test for calibration

e3c96e3

andrea-pasquale requested a review from Edoardo-Pedicillo October 29, 2024 14:42

chore: Add TODO

e5a49cd

andrea-pasquale mentioned this pull request Oct 30, 2024

Porting some protocols to qibolab 0.2 #1036

Merged

8 tasks

feat: Adding dispersive shift

f4b7049

andrea-pasquale mentioned this pull request Nov 5, 2024

Add experiment for qubit frequency vs flux amplitude #1037

Open

Edoardo-Pedicillo approved these changes Nov 5, 2024

View reviewed changes

andrea-pasquale mentioned this pull request Nov 5, 2024

Update to qibolab 0.2 #990

Open

76 tasks

stavros11 merged commit a698464 into 0.2 Nov 7, 2024
10 of 19 checks passed

stavros11 deleted the 0.2-cal branch November 7, 2024 11:06

		from qibolab._core.identifier import QubitId, QubitPairId
		from qibolab._core.serialize import NdArray

	QubitId = Union[str, int]
	QubitPairId = tuple[QubitId, QubitId]

		# TODO: Add something related to resonator calibration


		class Qubit(Model):

Adding calibration parameters to qibocal #1021

Adding calibration parameters to qibocal #1021

Conversation

andrea-pasquale commented Oct 22, 2024 • edited Loading

alecandido commented Oct 22, 2024

andrea-pasquale commented Oct 22, 2024

andrea-pasquale commented Oct 25, 2024

alecandido commented Oct 25, 2024

andrea-pasquale commented Oct 28, 2024

alecandido commented Oct 28, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido commented Oct 28, 2024 • edited Loading

andrea-pasquale commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

stavros11 left a comment • edited Loading

Choose a reason for hiding this comment

alecandido commented Oct 28, 2024 • edited Loading

andrea-pasquale commented Oct 28, 2024

Edoardo-Pedicillo left a comment • edited Loading

Choose a reason for hiding this comment

Edoardo-Pedicillo Oct 28, 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

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

Edoardo-Pedicillo Oct 28, 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

andrea-pasquale commented Oct 28, 2024

alecandido commented Oct 28, 2024

stavros11 commented Oct 29, 2024

Edoardo-Pedicillo left a comment

Choose a reason for hiding this comment

alecandido commented Nov 7, 2024 • edited Loading

andrea-pasquale commented Oct 22, 2024 •

edited

Loading

alecandido commented Oct 28, 2024 •

edited

Loading

alecandido Oct 28, 2024 •

edited

Loading

stavros11 left a comment •

edited

Loading

alecandido commented Oct 28, 2024 •

edited

Loading

Edoardo-Pedicillo left a comment •

edited

Loading

Edoardo-Pedicillo Oct 28, 2024 •

edited

Loading

Edoardo-Pedicillo Oct 28, 2024 •

edited

Loading

alecandido commented Nov 7, 2024 •

edited

Loading