Closed alecandido closed 7 months ago
Before merging, it should be tested on the devices, just because there is a large surface not covered by the no-qpu tests (actual QPUs access is irrelevant, of course).
Moreover, this is breaking part of the PulseSequence
interface (.serial
, .hash
, and .add()
have been dropped, .copy()
changed meaning), so I'd rather collect this with #750 and prepare a breaking release (but only at the end of the pulse rework).
Attention: 7 lines
in your changes are missing coverage. Please review.
Comparison is base (
92f35df
) 62.45% compared to head (6d46ba9
) 61.99%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Also, it's a bit weird to see gate names in lowercase, but maybe it's just me.
Yeah, this was unrelated, but I wanted to do it at some point, and I started to do a piece at a time.
The idea is that there is a convention about variables naming, i.e. snake_case
, and I know there are some words that are not the best fit for it, and they look kind of weird, but then you would have to decide when exceptions are allowed and when not.
PEP 20 says:
Special cases aren't special enough to break the rules. Although practicality beats purity.
(emphasis mine)
In general, I decided to avoid breaking naming conventions completely, because it's really hard to decide where to put the threshold (so the simplest choice: no break).
The usually complicate case is with acronyms. And the simple answer is: treat acronyms as if they were words (thus an RX90 variable becomes rx90
- but there is some room: a corresponding class could be both Rx90
or RX90
).
@Jacfomg let me know what you think about variables names.
Otherwise, as soon as you and @stavros11 agree on the content, we could merge. I.e. I would not wait to make a coupled PR in Qibocal right now, but just to attempt the upgrade when we'll be close to merge the whole 0.2
branch.
Btw, once we'll be closer, we could even publish pre-releases for 0.2
, to avoid working against a moving target (but if the branch is enough, we could use just that).
@Jacfomg let me know what you think about variables names.
Coding wise it makes sense to change them for what you said, I was just used to them being in capital letter from other sources. I suppose you might also want to change the .create_RX_pulse
into .create_rx_pulse
in the future so they follow the same logic ?
Coding wise it makes sense to change them for what you said, I was just used to them being in capital letter from other sources. I suppose you might also want to change the
.create_RX_pulse
into.create_rx_pulse
in the future so they follow the same logic ?
Yes, but it doesn't have to happen all at once, also because some of them would break compatibility (since they are used outside).
I’m guessing that most cases where these methods are used outside,
sequence.add
is also used so it could even be a good idea to break (and fix) both together. However particularly forplatform.create_*
I would even consider dropping at some point, because it is just a duplicate interface for accessing native gates, which are already underQubit
. But this only after we finalize the structure of native, because to my understanding the currentQubit
structure is also questionable.
Indeed, I'm aiming to the create_*
method since some time, because as you say they are redundant, and they scale bad (it's not just one redundant method, but you need them for each gate).
But I agree with you, let's wait for native. We could do in 0.2 as well, but right now I want to focus just on pulses.
Thanks @alecandido, I agree with these changes. I get that the main change in this PR is the conversion of
PulseSequence
tolist
and removal of several methods, right? The rest are just updates for compatibility with this interface. If yes, feel free to merge.
Yes, that's correct (pulses are used everywhere, so these PRs always touch a lot of files...).
Since
PulseSequence
is implementing an extended portion of thelist
API, and not holding any other attribute, it is more convenient to directly make it alist
.The remotion of some methods will require some additional work, and it might break the compatibility with Qibocal as well:
__init__
can not be called with multiple arguments__eq__
and__ne__
are not checking any longer if it's an actualPulseSequence
__add__
does not support addingPulse
.append()
will have to be used for this, as with regular listsreturn PulseSequence(super().__add__(self, other))
, to fix the return object__radd__
(we support only homogeneous sum) and__iadd__
(works out of the box)__getitem__
should support the broader slice semantics, but it was not, and I won't do it as well.add()
has been dropped, in favor of.append()
,.extend()
, and__add__()
(as for regular lists).copy()
has changedcopy.deepcopy()
for the other onePulse.copy()
, in favor of a plain copySince it was not appropriate for a mutable object to be hashable,
__hash__
has been dropped.PulseSequence.serial
was mostly used for sequences comparison. But, in the optics ofPulseSequence
being a mere list, two sequences are the same if every pulse of the sequence is the same. It makes sense that the equality relation used is the defaultPulse.__eq__
. Anything more elaborate should be made explicit in user's code.