search

qiboteam / qibolab

Quantum hardware module and drivers for Qibo.
https://qibo.science
Apache License 2.0
41 stars 12 forks source link

Drop pulse serial #750

Closed alecandido closed 7 months ago

alecandido commented 7 months ago

The goal of this PR is to drop pulse.serial and its relatives, since their goal should have been serialization, but they were mainly used as a hash of the pulse itself.

To the best of my knowledge, they have never actually been used for serialization, so I'm not aiming to preserve this functionality. With the transition to dataclass, serializing the content should be pretty simple (as simple as calling asdict(), for as long as it is not nested), so this is something that we could do even later on (especially because many other things related to Pulse will change in the near future).

Pulse.__hash__ will automatically come from a frozen instance. But, to reduce the effects on everything else, I will avoid freezing Pulse for some time, and manually provide a hash (not really appropriate for a mutable object, but for as long as we do not use mutability and the hash at some time nothing terrible will happen). Thus, it is intended only as a placeholder.

The hash is needed because there are places where the pulses want to be compared according to their actual content, rather than a unique ID. The main use case will be deduplication, but it's already being used for comparing the content of sequences, so it is required to reproduce part of the behavior of Pulse.serial.

stavros11 commented 7 months ago

Thanks, I agree with the removal. Just two quick remarks without checking all the changes:

  1. Given that you are replacing with pulse.id, is that property even needed? In principle the user can do id(pulse) directly and get the same thing if they want. One could argue that every class in Python should provide the id property, but Python itself does it for us.
  2. You probably are aware already, but pulse.serial is used all over the place in qibocal and probably in every other script using qibolab to grab the acquisition results, so we should be careful with the version compatibility after merging this.
alecandido commented 7 months ago
  1. Given that you are replacing with pulse.id, is that property even needed? In principle the user can do id(pulse) directly and get the same thing if they want. One could argue that every class in Python should provide the id property, but Python itself does it for us.

This is something I'm genuinely unsure about. For the applications I have in mind, I'm even considering weakening id, to be less unique. Essentially, we should have:

I also want the second for the comparison, because if I copy the pulse I'd like to have a different id, but they should compare the same. Something like:

pulse_ = pulse.copy()
assert pulse == pulse_
assert pulse is not pulse_
assert pulse.id != pulse_.id

(even though it doesn't have to be called .copy()).

However, I'm not sure if a loaded pulse should actually be different from its original version. Let's say that I dump the results and the sequence, I might still want to be able to retrieve the content as loaded_results[loaded_pulse.id]. But with loaded_results[id(loaded_pulses)] this would be lost, because it's actually a new object in memory, so Python has no reason to assign again the same ID.

That's why I was thinking to replace the property with an actual attribute, to be initialized by __post_init__ with a UUID, and serialized together with the rest of the pulse (but not copied in deep copies...).

Maybe it's overly complicated, but that's what makes me unsure. And with pulse.id I'm always in time to change from the property to the attribute and vice versa, without having to touch everything every time.

alecandido commented 7 months ago
  1. You probably are aware already, but pulse.serial is used all over the place in qibocal and probably in every other script using qibolab to grab the acquisition results, so we should be careful with the version compatibility after merging this.

Oh, definitely!

Most of the changes in #683 are non-breaking. But a few of them are breaking, and these are some of those.

In principle, before merging we could provide a temporary pulse.serial alias of pulse.id, since I'm not replacing it, but just dropping. Then we can decouple the compatibility breaking from the pulse refactoring.

alecandido commented 7 months ago

This is on top of #756, so that one should be merged first (even though both will end up in 0.2).

codecov[bot] commented 7 months ago

Codecov Report

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

Comparison is base (8747b46) 61.99% compared to head (dea5405) 61.84%. Report is 28 commits behind head on 0.2.

Files Patch % Lines
src/qibolab/instruments/qblox/controller.py 0.00% 9 Missing :warning:
src/qibolab/instruments/qblox/cluster_qrm_rf.py 0.00% 4 Missing :warning:
src/qibolab/instruments/qm/sweepers.py 50.00% 4 Missing :warning:
src/qibolab/pulses.py 90.69% 4 Missing :warning:
src/qibolab/instruments/zhinst.py 0.00% 2 Missing :warning:
src/qibolab/platform.py 85.71% 2 Missing :warning:
src/qibolab/instruments/qblox/cluster_qcm_bb.py 0.00% 1 Missing :warning:
src/qibolab/instruments/qblox/cluster_qcm_rf.py 0.00% 1 Missing :warning:
src/qibolab/instruments/qm/driver.py 0.00% 1 Missing :warning:
src/qibolab/instruments/rfsoc/driver.py 66.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## 0.2 #750 +/- ## ========================================== - Coverage 61.99% 61.84% -0.16% ========================================== Files 47 47 Lines 5776 5748 -28 ========================================== - Hits 3581 3555 -26 + Misses 2195 2193 -2 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/750/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qiboteam/qibolab/pull/750/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `61.84% <80.53%> (-0.16%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alecandido commented 7 months ago
  1. about pulse.id not being very useful as it is, as it is equivalent to id(pulse), but I understand that you'd like to keep it as property because you intend to change its implementation at some point.

Exactly

2. the debugging issue. This is QM specific, I am not sure if it is relevant for other drivers, but until we have a proper mechanism for debugging (which I am not really sure how it would look), I would rather keep the QUA scripts readable.

Ok, I take your point. Let's open an issue to solve before merging 0.2, rather than inflating this PR. The moment the Pulse dataclass will be completed, it will be always possible to retrieve the whole pulse given its ID, and you could get a representation of the Pulse from the one provided by dataclasses by default. But I would rather double the QUA scripts, or have a flag to generate a readable one. Outside the QM driver, I would make Qibolab to reason just with IDs, for as long as you do not need any parameter.

Also, probably not for this PR, but the Waveform class looks a bit useless. It is a wrapper over a numpy array and implements trivial methods like __len__, __hash__, etc.

That class is already dead in my mind. I want to replace that and the PulseShape as well:

So, yes, I definitely agree with you.

alecandido commented 7 months ago

Btw, I'm not in an extreme hurry to merge, but I want to keep 0.2 up to date, and I don't want to propagate changes to a bunch of branches.

That's why I'm planning to merge these PRs even with a single approval (only main is protected with 2, so there is no technical issue).

I will wait until Tuesday. If you don't have time @PiergiorgioButtarini no worries, it's simply not necessary. In case, #768 has more priority (just because you're the only reviewer).

PiergiorgioButtarini commented 7 months ago

Thanks @alecandido! From my side I don't need a readable identifier (like it was Pulse.serial), so everything looks good to me. Only problem dropping waveform.serial will be a breaking changes for Qblox and I will open an issue in order to solve it asap.

alecandido commented 7 months ago

There will be quite a few breaking changes, but consider that I'm merging to 0.2, not to main, and we will a dedicated round of drivers fixing and testing. No need to rush :)

stavros11 commented 7 months ago

Thanks @alecandido! From my side I don't need a readable identifier (like it was Pulse.serial), so everything looks good to me. Only problem dropping waveform.serial will be a breaking changes for Qblox and I will open an issue in order to solve it asap.

@PiergiorgioButtarini just noting that Waveform was removed in #774 which now merged to the 0.2 branch, but you may still want to have a look. @alecandido did some changes on Qblox on that PR, not sure if these are what you meant. We have not tested any of this on hardware yet (at least myself).

alecandido commented 7 months ago

I didn't test as well, just waiting for merging to 0.2, and then test altogether.

On the one side, I'm not even sure we want to merge now, because maybe we want to add even more breaking changes before merging. For sure, the January release will be a 0.1.x, but maybe even the February one (that would mean to merge 0.2 at least in March).

EDIT: my plan is to keep 0.2 updated with main (I will rebase tomorrow) in any case, and start testing on hardware well before the full completion, just to have two Qibolab versions side-by-side for some time (even though not for too long, to avoid lots of merges and double implementations)