Trim pulse's `Shape` to envelopes

[alecandido]

The current PulseShape has quite a verbose implementation, but essentially it holds functions for the shape envelopes. I will try to retain only that, delegating everything else outside (including the determination of the number of samples).

• [x] deserialize pulses (former PulseShape.eval)
  • introduce pydantic
• [x] move amplitude out of envelopes
• [x] move duration out of envelopes (back to Pulse)
  • see https://github.com/qiboteam/qibolab/pull/818#issuecomment-2016380597
• drop PulseType
• [x] fix tests
• [x] fix doctests

There is also a further issue that has been discussed, but without a clear conclusion (https://github.com/qiboteam/qibolab/pull/818#discussion_r1513373435).

• [ ] take a decision about relative_phase

My personal take is that this is a modulation problem, since, for as long as the I and Q components are kept separate, and the information about relative_phase is retained, there is nothing lost and nothing to be done. Only when you want to mix them, you should consume the relative_phase info. In any case, it is definitely not intrinsically related to shapes/envelopes, that are the core of this PR, so I'd rather open a separate issue.

[alecandido]

@stavros11 and @hay-k I asked you a review immediately, such that you could comment on the overall strategy before I keep going to apply it to the whole file.

The new part is down to class Shapes, everything else still depends on the old PulseShape, and I haven't touched yet. But there are no surprises, it will be pretty straightforward (and it will collapse the number of lines in that file).

[alecandido]

@stavros11 and @hay-k, feature-wise the PR should be complete, now I need to fix the tests and propagate the changes.

If you'd start having a look you could already provide some feedback.

[alecandido]

Question: is there any case where times is not something like np.linspace(0, duration, nsamples) (where nsamples = sampling_rate * duration))? Is this array supposed to be generated in all drivers calling the pulse.i and pulse.q methods?

Thanks for asking: the honest answer is "I don't know". However, here I started with the assumption that I wanted to evaluate a function on some points. Then, I had to change my mind. And, in the last reshuffle, I even assumed a linspace, to use the scipy.signal.windows.gaussian. So, we should definitely decide to either assume a linspace everywhere (and possibly retain only the linspace parameters), or I should revert the usage of the SciPy function.

[alecandido]

The other point is about the way we are handling i and q, which seems to me as kind of reinventing complex numbers. Since we are using numpy, which supports complex numbers,

Concerning this, I have little experience with the drivers, so I don't know whether they are going to consume this information separately, or as complex numbers.

In any case, I believe it is valuable to keep .i() and .q() in Envelope, since they are usually defined as two independent real functions, rather than a single real-to-complex one. But we could collect them in a single complex number, and expose only that at the level of the Pulse (i.e. drop Pulse.i()/.q() and keep only Pulse.envelopes(), possibly renaming it - and drop Envelope.envelopes() as well, since it is duplicated in Pulse, and definitely not required at low level).

[stavros11]

Concerning this, I have little experience with the drivers, so I don't know whether they are going to consume this information separately, or as complex numbers.

Probably most instruments instruments do not support uploading complex waveforms (but maybe ZI does?), but even in that case I would be fine doing

waveform = pulse.envelope(...)
upload_to_instrument...({"i": waveform.real, "q": waveform.imag})

over

upload_to_instrument...({"i": pulse.i(...), "q": pulse.q(...)})

if the former helps simplifying other parts of qibolab.

[alecandido]

waveform = pulse.envelopes(...)
upload_to_instrument...({"i": waveform[0], "q": waveform[1]})

This is already possible. We could follow PEP 20 and drop an alternative.

However, I'm not sure whether a shape=(n,), dtype=complex array is any better than a shape=(2,n), dtype=float in this case.

[stavros11]

However, I'm not sure whether a shape=(n,), dtype=complex array is any better than a shape=(2,n), dtype=float in this case.

Yes, I am aware of pulse.envelopes(...) and I am also not sure about which one is better (they may even be equivalent). The main advantage of complex is that it can simplify some manipulations we are doing, such as the modulation/demodulation functions (I think we can get rid of einsum if we use complex) or the transformation in my comment above about the phase (if we decide to implement it). Ideally, we would also like to be consistent with the results and use real or complex in both places.

[alecandido]

@stavros11 usually complex numbers are useful in electronics, because it's simpler to integrate/derive exponentials, and it's easier to compose them (multiplication would just sum the exponents). But I'm not sure we would leverage any of these niceties in our code, and if we're just working with real and complex parts separately, it's just simpler to use two arrays (concerning mod/demod you are right, since sine and cosine could be combined in a single exponential, so we'd get rid of one dimension - but it's only relevant for software modulation, and I'd say that is already simple enough...)

[alecandido]

@hay-k @stavros11: to me, the current layout of pulses.shape is final. I'm now tempted to just fix the tests and merge (of course I will ask you to review after fixing the tests).

If you have any further comment about the strategy, now it's the best timing :)

I applied @stavros11 proposal concerning linspace, and I replied above about complex and relative phases. I'm not aware of anything else related to this subject, yet.

[alecandido]

I believe I achieved a new record:

========================================== 2103 failed, 126 passed, 94 skipped, 47 deselected, 3 xfailed, 3 xpassed, 3 warnings in 108.28s (0:01:48) ==========================================

(i.e. everyone depended on PulseShape, ...)

[alecandido]

I'm going to fix the tests in a quick and dirty way (the least dirty the best, of course). The message is: I'll delegate tests improvements to everyone that is going to improve the corresponding source, as more competent on the matter (I believe is a sensible attitude for every PR, since the goal is minimizing the changes).

If anyone at any time (mainly @stavros11 and @hay-k) has a chance to decouple some tests from the pulses subpackage, please do your best. Otherwise, further improvements will always be a mess...

Instead, I'm going to take care of shapes-specific tests.

[alecandido]

I'm actually keeping the same signature as before for pulses envelopes, since sampling_rate is the only information missing to a pulse to determine which samples to take. And it is truly external, so it should be provided just for the envelope computation (instead of stored by the pulse, since the exact same pulse would allow different sampling rates).

However:

• I'm not applying any default for the sampling rate, since there is no default machine (dummy can define its own, as it is doing)
• the envelopes will still ask for the Times (i.e. duration and number of samples), since that's the required information - the Pulse can generate it given the sampling rate

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 89.92537% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 48.75%. Comparing base (6790872) to head (6f4c84b).

Files	Patch %	Lines
src/qibolab/instruments/zhinst/pulse.py	0.00%	10 Missing :warning:
src/qibolab/pulses/envelope.py	93.75%	8 Missing :warning:
src/qibolab/serialize_.py	84.84%	5 Missing :warning:
src/qibolab/instruments/qm/sequence.py	25.00%	3 Missing :warning:
src/qibolab/serialize.py	88.88%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## 0.2 #818 +/- ## ========================================== - Coverage 49.52% 48.75% -0.77% ========================================== Files 59 61 +2 Lines 5642 5532 -110 ========================================== - Hits 2794 2697 -97 + Misses 2848 2835 -13 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/818/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/818/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `48.75% <89.92%> (-0.77%)` | :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]

As agreed with @stavros11 and @hay-k, I'm now targeting #789

[alecandido]

@stavros11 @hay-k in the last commit (61ffcf8) I moved duration from Pulse to Envelope, as we discussed.

However, I now believe this to be an error: the Envelope does not care at all (or almost) about the duration, or better, that duration. The reason is that there is only one unit that the envelope needs to care about: samples. So, the duration in ns is not relevant for the envelope, but only for the pulse and whoever is doing the modulation (such that envelopes in samples and frequencies in GHz could be reconciled).

Now, the samples are only known when the sampling rate is known, and we said that we don't want to encode this information in the object, because the sampling rate (and thus the number of samples, i.e. samples) is instrument-specific, while the same envelope (and pulse) could be used by different instruments. That is why I kept samples as the only argument of the .i() and .q() methods.

Caveat: there are a few pulses that, by accident (or suboptimal design), actually depend on the duration. This is only by virtue of some parameters being expressed in ns, or derived units. These are:

• Exponential, where tau and upsilon are suboptimal (expressed in ns) and with a wrong description (they are described as decay rates, but they are actually lifetimes)
• GaussianSquare, where the width is expressed in ns
• Snz, where t_idling is expressed in ns
• ECap, where alpha is expressed in GHz

On top of being inconvenient for the object isolation and modularity, they are also inconsistent with another common and very relevant parameter: rel_sigma of Gaussian and all the Gaussian-like pulses, that is expressed relative to the overall duration (i.e. 0.5 is 50% of the duration, being it in ns or samples). Expressing this parameters in samples would make the envelopes (and thus pulses) not portable from one instrument to the other (not that this portability is extremely useful, since you would recalibrate anyhow, but it'd be a good starting point for the new calibration). But the rel_sigma pattern is portable and convenient, I'd just apply it to all parameters.

[alecandido]

@stavros11 @hay-k: I still have to fix the doctests, but at least Pytest is passing, so you could start the "final" review.

In the end, I decided to postpone the PulseType removal, mainly because I realized that Pulse.channel is treated as optional (i.e. it is not always set), and we have some inconsistencies about qubit vs couplers (i.e. at some point you get a number, and you're not sure if that number is the ID of a qubit or a coupler, unless sometimes where the qubit attribute is not used, and it's dynamically removed for an undeclared coupler attribute, especially in serialization).

So, we know PulseType to be redundant, but it requires some non-trivial standardization work to get rid of it, and this PR is bloated enough. I will write an issue about that.

[alecandido]

Ok, I just fixed the doctests, but not the docs themselves. And I believe they could be outdated since long...

After rolling out all new features, and having fixed the drivers, we should also review all the docs... (this could happen even after merging to main, at least we could make a pre-release and give Qibocal people some time to start updating it)

[alecandido]

@stavros11 @hay-k: here, https://github.com/qiboteam/qibolab/pull/818#issuecomment-2026771803, the note about the PulseType removal, collapsed by the latest rebase

[alecandido]

I don't get what exactly collapsed,

Since the commits have been pushed after the comment, and the PR got long, GitHub hid some part of it:

including that comment. That's why I linked, to quickly navigate ^^

[alecandido]

Another point that we should probably start thinking is to what extend we can drop serialize.py. I guess this is also a TODO since the new serialize_.py is a temporary name, right? A big part of serialize.py is trying to dump the platform in a fixed runcard format (mainly for historical reasons?) but maybe we could relax this requirement in favor of simpler serialization methods.

Yes, this I'm trying to achieve in an incremental way. I don't have an issue opened for that (maybe I should), but I believe it will gradually happen by simplifying the current serialize.py.

My current plan is to have a root object (the Platform), holding all the required settings, registered through Pydantic, such that the serialization will be platform.model_dump(). If that will happen, the serialization helpers will be just Pydantic invocations (for the whole object or individual components), and the current serialize_.py, containing Pydantic related utilities, could drop the underscore. However, I'm not sure if the plan will fully work as I thought, and I wanted to keep the old and new fashion well separate, though they are used together. I'd propose to finally reassess this before merging 0.2, but as late as possible.

[alecandido]

I don't like "kind" much, as a name choice. I believe "type" would read better in the runcard but it is also a Python keyword so it won't look nice in editors. "shape" would also read better but you may argue it is repeating "envelope"(?). Overally it is a matter of preference and I don't have strong opinion.

I agree that type would have been the name of choice. But it was so good that Python took it before... I'm using kind just as a synonym of type. However, it will be simple to replace, since it's easy to identify.

Personally I am fine with the change of rel_sigma from 5 to 0.2 and I agree that namewise it makes more sense, but we should be prepared to document and communicate it properly (among all other 0.2 things)

About this point, I agree and disagree. For sure, we'll need to help the Qibocal people transitioning to 0.2, since it will be non-trivial. However, the transition will look almost like a rewriting, since wherever there will be import qibolab or from qibolab you will have to do something different. rel_sigma is a bit special in this sense: it's the one of the few things that is breaking but not crashing. So, yes, we should remember to advertise this properly.

I am not very familiar with pydantic, so I am not sure if making there would be any advantage in propagating it to native.py as well. Certainly not for this PR, we can consider as we work on the serialization. Having these containers is also debatable because some people have requested a more flexible API for adding native gates (that are not CZs and CNOTs) externally.

I would move everything to Pydantic eventually. You could just consider it an upgraded version of the dataclasses. The price we had to pay is the external dependency. But, once it is there, I would use it as much as possible.

About the natives containers, at some point we should consider turning them into dictionaries, possibly using enums as keys (to limit the amount of possible gates, at least type-wise) and corresponding classes as values. It will be more or less the same, but easier to partially populate and easier to extend.

[stavros11]

@alecandido I just rebased 0.2 to main so we should do the same here. Let me know if you prefer to do it yourself, otherwise I can also do it. We could also consider merging this to 0.2 since many other open PRs already point to this branch and I would also like to start updating QM from this.

[alecandido]

@alecandido I just rebased 0.2 to main so we should do the same here. Let me know if you prefer to do it yourself, otherwise I can also do it.

I will try to rebase. If I'll notice that most of the conflicts are coming from places where you are more experienced than me, I will ask you to do it.

We could also consider merging this to 0.2 since many other open PRs already point to this branch and I would also like to start updating QM from this.

Fine by me, I was just waiting on @hay-k's review, to give him a chance to comment before adding to the large pot of 0.2 changes.

[alecandido]

Just rebased. I will merge as soon as the tests pass.