PulseTarget protocol and reference implementation#12096
Conversation
|
One or more of the the following people are requested to review this:
|
wshanks
left a comment
wshanks
left a comment
There was a problem hiding this comment.
This seems reasonable for reporting port logical functions (qubit, coupler) and standard frames. We might not want to limit the total number of frames per port.
I also wonder if we should think a little bit about how port-specific hardware constraints would be reported and see if we would change anything about the structure before accepting this.
| self, | ||
| qubit_frames: dict[int, str] | None = None, | ||
| meas_frames: dict[int, str] | None = None, | ||
| qubit_ports: dict[int, str] | None = None, |
There was a problem hiding this comment.
I would accept meas_ports as well to be more generic.
There was a problem hiding this comment.
Yes, that makes sense. PR is updated but in more general fashion.
| 2: "OnlyPort", | ||
| }, | ||
| coupler_ports={ | ||
| (0, 1): "PortA", |
There was a problem hiding this comment.
For non-directed couplers, would you put both directions here?
| "This hardware doesn't provide any frame for " | ||
| f"MeasurementFrame of index {frame.index}." | ||
| ) from ex | ||
| raise TypeError( |
There was a problem hiding this comment.
I see how the framework here allows for the pulse compiler to map Qubit and Coupler logical elements and QubitFrame and MeasurementFrame frames to hardware port and frame identifiers. What about defining additional logical elements or frames? This framewrok lists the port and frame identifiers, but how should the user set up the mapping? Does the user use the indentifiers from PulseTarget to create Port and GenericFrame instances with those identifiers as names to create MixedFrames and then the compiler will translate those directly without look up with the PulseTarget methods? Or should this method also have a clause for GenericFrame that returns frame.name, and should get_port_identifier similarly return port.name for a Port input?
There was a problem hiding this comment.
I updated the model in 214185d. GenericFrame and Port are also considered. Since end-user cannot define ports, there is validation for port UID inside target. On the other hand, there is no UID validation for frame since I assume they can use arbitrary name. Of course there is limitation in number of available hardware resources, but this is something compiler pass must validate.
wshanks
left a comment
wshanks
left a comment
There was a problem hiding this comment.
I like the direction of the changes. I still wonder about the balance between matching the model and keeping the hardware model generic.
| available for this port to form a mixed frame. | ||
|
|
||
| tx_ports: A list of dictionary representing a spec of transmission ports. | ||
| The spec dictionary must contain "qubits", "op_type", "num_frames", |
There was a problem hiding this comment.
Maybe this should be a class/protocol? I don't want to make it hard to extend the structure but it would be good to capture required fields for developers to reference.
There was a problem hiding this comment.
Good suggestion. It's turned into dataclass.
| try: | ||
| return self._qubit_ports[port.qubit_index] | ||
| except KeyError as ex: | ||
| return self._get_port_common(signal_entry, "generic") |
There was a problem hiding this comment.
I wonder if _get_port_common should be a public interface, maybe with a default value for op_type? It seems awkward to use a special method for measure ports and another for everything else. Also, it feels like a language mismatch to label the ports "generic" and "measure" while the frames are "Qubit", "Measure" and "Generic" -- for one qubits are "generic" but for the other they are distinct from "Generic". Maybe the qubit port op types should be "control" and "measure" or something like that?
There was a problem hiding this comment.
This is reasonable suggestion. Two methods are merged into a single method get_port_identifier which is aligned with frame query get_frame_identifier.
|
After Qiskit/RFCs#73 closing this PR. |
4 participants
Summary
This PR adds Qiskit Target model for pulse domain,
PulseTarget, and its reference implementationQiskitPulseTarget.Details and comments
Currently PulseTarget only defines APIs that map Qiskit pulse model to hardware resources. This could be extended to include more information, such as generic parametric pulse schema, available frequency range of frames, and supported instruction types. But these are as-needed basis. Vendor can also define own Target class based on the protocol, that provides necessary information for vendor specific compiler passes. Qiskit implementation is bare minimum to support builtin pulse compiler passes in Qiskit.