search

qiboteam / qibolab

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

Remove pulse creation helpers #793

Closed alecandido closed 4 days ago

alecandido commented 5 months ago

Most of the pulse creation boilerplate is just a different way to access the native pulses (in a single call, with more arguments, just saving some accessors).

The best approach for this would be to pass around Qibolab objects in Qibocal, instead of working with bare strings and integers all the way down, and then have to repeat the qubit object-retrieval procedure in each and every routine.

We could further simplify the access to native gates. But this is not strictly related to the creation methods themselves (but rather to the overall reorganization of natives).

alecandido commented 5 months ago

@andrea-pasquale here your opinion would be for sure relevant.

stavros11 commented 5 months ago

Quick note that may affect this: in #789 I simplified a lot native.py, which also slightly affects how these methods look https://github.com/qiboteam/qibolab/pull/789/commits/d279ba63eb24c1b083ec5efb81d5daaa80b01b6f. I didn't have time to summarize the changes in the other PR, I will do so tomorrow.

Other than that, personally I am not 100% sure what we should do with the platform.create_* methods. I certainly don't like their current names (are quite long) and there is also duplication and bad scaling issues, however I am also not fully convinced that the alternative API

platform.qubits[qubit].native_gates.RX

is very user-friendly.

alecandido commented 5 months ago

My idea is that in Qibocal, the qubit are serialized by name, but loaded as objects as soon as the platform is available (so not inside the routine, but before).

Thus, the user should do:

qubit.native_gates.RX

which is not unreasonable (to me).

andrea-pasquale commented 5 months ago

@andrea-pasquale here your opinion would be for sure relevant.

From the user point of view using directly native_gates is fine. I would mostly worry about the create_pulse_* methods related not to native_gates. What is the proposal for handling those? (Just for context I am mostly thinking about the FluxPulse which was related to https://github.com/qiboteam/qibolab/pull/771)

stavros11 commented 5 months ago

From the user point of view using directly native_gates is fine. I would mostly worry about the create_pulse_* methods related not to native_gates. What is the proposal for handling those? (Just for context I am mostly thinking about the FluxPulse which was related to #771)

I think in this case defining the pulse directly using the pulse API (as Pulse(amplitude, duration, ...)) is probably the simplest. Keep in mind that pulses will be much simpler to initialize in 0.2, as we are dropping start, qubit, type and most likely channel too.

alecandido commented 5 months ago

I would mostly worry about the create_pulse_* methods related not to native_gates. What is the proposal for handling those? (Just for context I am mostly thinking about the FluxPulse which was related to #771)

Consider that the FluxPulse does not exist any longer, so you will always have to specify on which channel you want to play your pulse.

andrea-pasquale commented 5 months ago

I think in this case defining the pulse directly using the pulse API (as Pulse(amplitude, duration, ...)) is probably the simplest. Keep in mind that pulses will be much simpler to initialize in 0.2, as we are dropping start, qubit, type and most likely channel too.

Great, if this is the case then I think that we can drop them. Btw, should we have also a corresponding development branch in qibocal to be synchronized with 0.2 branch in qibolab?

alecandido commented 5 months ago

Btw, should we have also a corresponding development branch in qibocal to be synchronized with 0.2 branch in qibolab?

At some point, definitely.

However, right now we're very much work-in-progress, and it's not planned to be merged before a couple of months. So, you'd do it at risk of updating it many many times. My advice would be to postpone this until we're quite closer to the final version, and thus even more stable (of course with some time in advance, to have the time to apply all the required changes in Qibocal).

andrea-pasquale commented 5 months ago

Btw, should we have also a corresponding development branch in qibocal to be synchronized with 0.2 branch in qibolab?

At some point, definitely.

However, right now we're very much work-in-progress, and it's not planned to be merged before a couple of months. So, you'd do it at risk of updating it many many times. My advice would be to postpone this until we're quite closer to the final version, and thus even more stable (of course with some time in advance, to have the time to apply all the required changes in Qibocal).

Ok, then just let me know when I should start updating Qibocal :)

alecandido commented 5 months ago

@stavros11 @andrea-pasquale is there enough consensus for this? Should I keep going and fix tests?

andrea-pasquale commented 5 months ago

@stavros11 @andrea-pasquale is there enough consensus for this? Should I keep going and fix tests?

From my side we can proceed.

stavros11 commented 5 months ago

@stavros11 @andrea-pasquale is there enough consensus for this? Should I keep going and fix tests?

Fine for me too. Note that these methods will even lose one argument in #789 since we are dropping the start, which I guess makes them even less useful.

alecandido commented 5 months ago

Fine for me too. Note that these methods will even lose one argument in #789 since we are dropping the start, which I guess makes them even less useful.

Since this is such a simple change, but fixing the test will require quite a bunch of work (I guess), I'm actually tempted to wait for #789 before fixing. Maybe I will just focus on improving shapes in the meanwhile.

alecandido commented 3 months ago

Ok, since #789 has been merged, I started fixing the tests in this PR.

However, I immediately realized that there will be quite a "huge" overhead due to channel setting (every time you create a pulse, you should also reset the channel, since the default one could be the wrong one).

One option could have been to ensure that the channel was properly set during the native deserialization and updates. However, I decided that it's not worth it, since we're going to drop it anyhow in #792 (if I understood correctly, there is enough consensus, i.e. @stavros11, @hay-k, and me, to remove the .channel attribute from the pulse).

So, this PR is again on hold, waiting for #792 (after that, the replacement should be simple enough, as it was planned before).

stavros11 commented 1 week ago

Just noting that this is also being done in #885, I am guessing independently from here. A different representation of natives is also introduced there.

alecandido commented 1 week ago

Just noting that this is also being done in #885, I am guessing independently from here. A different representation of natives is also introduced there.

Thanks @stavros11. As soon as we'll start the review of #885, I will consider closing this.

Here there is just one trivial commit, but I do not bother to open a dedicated issue now. I'd be happy to close this if it's done somewhere else (this commit is here since the January 30, but waiting for the channels to fix the tests).

alecandido commented 4 days ago

Ok, better the issue at this point.