Drop pulse serial

[alecandido]

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.

• drop Pulse.serial
• drop PulseSequence.serial
  • done in PulseSequence to list #756
• drop Waveform.serial
• replace pulse.serial with pulse.id (most places)
• add Pulse.__hash__
• replace part of pulse.serial with hash(pulse)

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]

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]

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:

• a fully unique ID for each pulse (more on this after)
• a hash, representing the whole content (this will be provided by the frozen dataclass)
• potentially weakened hashes, taking into account only part of the attributes (for specific applications, like dedup)

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]

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.

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]

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

[codecov]

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 ⚠️
src/qibolab/instruments/qblox/cluster_qrm_rf.py	0.00%	4 Missing ⚠️
src/qibolab/instruments/qm/sweepers.py	50.00%	4 Missing ⚠️
src/qibolab/pulses.py	90.69%	4 Missing ⚠️
src/qibolab/instruments/zhinst.py	0.00%	2 Missing ⚠️
src/qibolab/platform.py	85.71%	2 Missing ⚠️
src/qibolab/instruments/qblox/cluster_qcm_bb.py	0.00%	1 Missing ⚠️
src/qibolab/instruments/qblox/cluster_qcm_rf.py	0.00%	1 Missing ⚠️
src/qibolab/instruments/qm/driver.py	0.00%	1 Missing ⚠️
src/qibolab/instruments/rfsoc/driver.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            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	Coverage Δ
unittests	61.84% <80.53%> (-0.16%)	⬇️

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]

Thanks, changes are fine with me. My only concerns are what I raised before:

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.
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. This is particularly because I have an open communication with some people from QM where I send scripts generated by the driver and they check the quality, so it is probably better if they are somehow readable. This to keep in mind while fixing conflicts with Sequence unrolling with QM #738.

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.

[stavros11]

I did not test, but there is a possibility that this will not work as it will need str in both sides. Again, this will become a problem while trying to fix the conflicts between this and #738.

[alecandido]

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:

• a waveform is nothing else than an array, since it's what we could send to the FPGA
• a PulseShape should be a pair of callable (I and Q envelopes) getting some parameters and a time - I'm just deciding if I should keep the class to store the parameters, or whether I should pass the parameters directly to the callables

So, yes, I definitely agree with you.

[alecandido]

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]

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]

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]

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]

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)