-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Qiskit Pulse - Introduce logical elements and frames #10694
Conversation
One or more of the the following people are requested to review this:
|
…iskit-terra into LogicalElementsFrames
Pull Request Test Coverage Report for Build 6526630111
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TsafrirA. This is good amount of code to carefully review. I'm glad to see finally we came to implementation phase. I feel logical element is one of the great inventions from our long discussion :)
I started to reconsider the handling of name property in Qiskit, after our internal discussion about mixed frame identifier mapping. I lean toward using only type information in Python layer, and later assign string identifier tailored for each backend system configuration when we create a communication payload. This requires slight design change in frame and logical elements, but I think this will remove complexity of mixed frame notation mapping between Qiskit and backend compiler.
that qubits are coupled. | ||
""" | ||
|
||
def __init__(self, logical_element: LogicalElement, frame: Frame): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you turn logical_element into optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's a good option. A MixedFrame with no LogicalElement is simply a Frame, and it will be less confusing to just use the Frame. This will convey in a clearer way the meaning of the two. For example, on the instruction level, a user can choose to apply a shift phase instruction to a Frame instead of a MixedFrame, but that would clearly indicate that the instruction will be broadcasted across MixedFrame's.
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
@nkanazawa1989 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Tsafrir for your great work! This is almost good for merge. My review is bit nitpicky this time. Since this is introduction of new module, I'm trying to minimize a risk of future deprecation.
Another point I would consider is the use of slots. This optimizes memory footprint. I remember one experienced researcher worried about performance bottleneck due to heavy usage of python objects. Proper use of slots will alleviates such performance overhead.
""" | ||
self._logical_element = logical_element | ||
self._frame = frame | ||
self._hash = hash((self._logical_element, self._frame)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason of having this _hash attribute? Just a curiosity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following the logic of the Channel module. I think it makes sense to have this here as well, since we'll probably want to key things on the MixedFrame, and we can thus hash everything and do it only once. I can remove that if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I added some suggestions for the docstrings.
The classes could just about be dataclasses though maybe it's not worth the extra features they bring along.
qiskit/pulse/model/__init__.py
Outdated
|
||
MixedFrame | ||
============= | ||
The combination of a :class:`LogicalElement` and :class:`Frame` is dubbed a :class:`MixedFrame`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could some intuition for this name be given? Does "mixed" refer to the output of a microwave mixer or the combination of two kinds of frame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great question I can't answer. @nkanazawa1989 any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is conventional name and I don't know how this was introduced. Probably we can introduce another name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here "mixed" doesn't refer to the microwave mixer. 'MixedFrame' refers to a frame that is associated with a LogicalElement
vs Frame
which is not associated
better convey the meaning of the code, and change the compilation process. One example | ||
are shift/set frequency/phase instructions which are not broadcasted to other | ||
:class:`MixedFrame`s if applied on a specific :class:`MixedFrame` (unlike the behavior | ||
of :class:`Frame`). User can also use a subclass of :class:`MixedFrame` for a particular |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do set/shift instructions work differently for MixedFrame
compared to Frame
? Is there a specific use case motivating that behavior, over applying the instruction to all MixedFrame
s sharing the Frame
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here is to leave all options open for the user. If you want to target a specific MixedFrame
- you can (the same as you would target a specific Channel
before). But, if you want to target all MixedFrame
s associated with a Frame
you can do that too by targeting the Frame
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, the old Channel
s are effectively instances of MixedFrame
s (and might become subclasses concretely?), so this preserves backwards compatibility.
What I was thinking about when asking about this is that I was thinking it made the implementation more complex since both frames and mixed frames need to be tracked in PulseIR (or other any representation). I think the way it has to work is that a frame is an alias for all of its mixed frames. So one way to think about it is that all shift/set frequency/phase instructions on a frame are replaced with those instructions on all mixed frames in a first pulse compiler pass. There is not really a standalone frame that has a frequency and phase over time, though you can think of there being one if you never apply instructions directly to mixed frames that make them different from the underlying frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the endgame is to deprecate the Channel
s.
Yes, one of the first passes done by the compiler would be to broadcast instructions across MixedFrame
s. In principle, you can also imagine broadcasting instructions across MixedFrame
s associated with a specific LogicalElement
- I think a delay is the only reasonable example?
qiskit/pulse/model/__init__.py
Outdated
to define where pulses instructions are applied, and what would be their carrier frequency and phase | ||
(because typically AC pulses are used). Each :class:`LogicalElement` represents a separate component | ||
in the quantum system on which instructions could be applied. On the other hand, each :class:`Frame` | ||
represents a frequency and phase duo for the carrier of the pulse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to be more careful here. In the Frame
class, you specify initial frequency and initial phase. The Frame
also represents the object to reference to change the frequency and phase duo over time by applying frequency and phase instructions but that frequency and phase duo can only be determined by scheduling a pulse program.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this is a delicate topic.
My thinking was that we need to set the initial frequency and phase at time t=0
for Frame
s that don't have a default value taken from the backend. If you think of a pulse program (like the old Schedule
) this is sufficient. Things might get trickier if we think of a program with several pulse gates. In principle the initial values are still enough, but you will have to track the phases across the entire program? (To be fair, this is not handled well even with the current stack)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was something else I was thinking about how to ask about. One of the motivations for this work on pulse frames was to help with the qutrit and cross-resonance cases where there is a need to apply a shift and have it persist across pulse gates. Do we try to capture that here? Or is that left to the backend implementation? At some point, we need to introduce a way to change the IBM backend implementation so that a pulse gate can influence later pulse gates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great question, which we haven't ironed out yet.
I think the user friendly approach would be to define the initial frequency and phase at t=0
where t=0
is not the beginning of the pulse gate, but rather the beginning of the circuit. This gives a concrete meaning to the frame which could then be tracked over time. The job of actually tracking such a frame would have to be assigned to either the backend or the frontend. However, this introduces a global dependency to the pulse gates which is not currently available in the stack. Perhaps this global dependency is needed anyway if we want to carry out phases and frequencies correctly across pulse gates? If that's the case, we can sort all of these issues via a compiler run which will schedule the circuit as a whole, and address the frames on a global basis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think eventually the management of frame is delegated to the backend. Frame is a global static variable and shared among pulse gates, and local shift/set frequency instruction doesn't update frame value.
Do we try to capture that here? Or is that left to the backend implementation?
Qiskit doesn't define how things must be implemented on the physical layer, so it's just a syntax. Actual implementation must be done on the backend, still IBM provider can provider a plug-in pass that computes local frame shift in each pulse gate (provider pulg-in pass should know backend implementation). If you run the same program on Qiskit Dynamics, phase and frequency are already carried over because it schedules entire circuit before running simulation. This is why I prefer pass based architecture.
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TsafrirA added fix for minor docs error. I'll approve and merge after the fix is applied.
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
…iskit-terra into LogicalElementsFrames
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This looks good to me.
Summary
This PR is the first step in implementing RFC0012. It introduces the new building blocks
LogicalElement
andFrame
to the pulse stack, which will take over the legacyChannel
s.Details and comments
For more details, see the RFC.
One element which is currently missing is a substitute for
MemorySlot
. The new class should play exactly the same role asMemorySlot
but we want it in the new namespace, while also not duplicating the name.Release notes will be added at a later stage for the combined changes.