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).

• deserialize pulses (former PulseShape.eval)
  • introduce pydantic
• move amplitude out of envelopes
• move duration out of envelopes (back to Pulse)
  • see Trim pulse's Shape to envelopes #818 (comment)
• drop PulseType
• fix tests
• fix doctests

There is also a further issue that has been discussed, but without a clear conclusion (#818 (comment)).

• 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.

[stavros11]

Thanks @alecandido. One question and another point from my side.

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?

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, maybe we could simplify by returing the envelopes/shapes as complex numbers and let the user do .real/.imag to grab the i/qcomponent respectively. For example the modulate operation would look much simpler:

return np.exp(1j * phases) * envelope / np.sqrt(2)

and we would not have to define three functions (i, q, envelopes) everywhere.

In 0.1 we are already using complex in the results

qibolab/src/qibolab/result.py

Line 18 in 5953517

self.voltage: npt.NDArray[np.complex128] = data

which is already asymmetric to the pulse API (however that's fair since we have not discussed results on 0.2 yet).

[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]

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 ⚠️
src/qibolab/pulses/envelope.py	93.75%	8 Missing ⚠️
src/qibolab/serialize_.py	84.84%	5 Missing ⚠️
src/qibolab/instruments/qm/sequence.py	25.00%	3 Missing ⚠️
src/qibolab/serialize.py	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            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	Coverage Δ
unittests	48.75% <89.92%> (-0.77%)	⬇️

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.

[alecandido]

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