-
Notifications
You must be signed in to change notification settings - Fork 10
✨ Pulse representation specifications #182
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
base: develop
Are you sure you want to change the base?
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #182 +/- ##
=========================================
- Coverage 90.8% 90.6% -0.2%
=========================================
Files 7 7
Lines 1072 1071 -1
Branches 222 221 -1
=========================================
- Hits 974 971 -3
- Misses 98 100 +2
see 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Hi @donsano33 and @amitQC, |
ystade
left a comment
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 @mnfarooqi for this first proposal for the pulse level support. Your implementation is already very clean and follows the look and feel of QDMI.
I went though it skipping the FoMaC implementation for now because I think, we should first dicuss some high level topics first. In a later iteration, we can also consolidate FoMaC and the testing.
As indicated in one of the comments, I have one major point of concern: With QDMI_Pulse_Waveform we are defining our own standard for representing pulses. Honestly, I am not an expert in pulse level quantum computing. However, I strongly argue that introducing a new calibration grammar should be well-considered. This concern accumulates in the following two points:
- The current proposal allows to query the waveform in the OpenPulse format. Why do I then even need to have another representation as OpenPulse seems to be supported. I suggest that we decide to use one representation and not multiple to keep the interface minimalistic and do not impose additional implemenation efforts on device implementers.
- The current (QDMI) waveform specification relies on an unspecified string. First of all, I would argue that a string to represent a term is not the best choice. However, what I find more important to fix the semantics of the pulse representation. Right now, a compiler cannot expect anything from the returned string. For example, consider, the OpenPulse Grammar. If we do not want to rely on open pulse (for whatever reason), we need a semantic for our pulse representation as well.
@mnfarooqi sorry, questioning the pulse representation. This has nothing to do with the actual code. Your code is perfectly well-strucutred, well-documented, and clean. Nevertheless, we should consolidate what we actually want here.
| #include "c_qdmi/types.h" | ||
|
|
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.
| #include "c_qdmi/types.h" |
Iirc, this should not be necessary since the header above already re-exposes the types header.
| struct C_QDMI_Pulse_Waveform_impl_t {}; | ||
| struct C_QDMI_Pulse_Implementation_impl_t {}; |
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 types, i.e., the structure names ending with _t should not needed to be redefined by the device, actually, this should lead to a compiler error because of redefinition. Am I overseeing something?
| C_QDMI_Device_Session session, C_QDMI_Pulse_Parameter param, | ||
| const QDMI_Pulse_Parameter_Property prop, const size_t size, void *value, | ||
| size_t *size_ret) { | ||
|
|
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.
Even if no property is supported, the function is expected to validate its arguments and return QDMI_ERROR_INVALIDARGUMENT if necessary. That has higher priority than QDMI_ERROR_NOTSUPPORTED.
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 comment also applies to the two subsequent functions.
| #include "cxx_qdmi/types.h" | ||
| #include "qdmi/constants.h" |
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.
Those should all be re-exported by the device header and not needed here. In particular, including headers from the directory with the prefixed headers and from the main include directory does not seem clean to me.
| #include "cxx_qdmi/types.h" | |
| #include "qdmi/constants.h" |
| }; | ||
| } // namespace | ||
|
|
||
| const CXX_QDMI_Pulse_Parameter_impl_d AMPLITUDE{"A"}; |
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.
If I understand that correctly, than only "A" is transferred as the name of the parameter through QDMI. Is that standardized anywhere? If not, I propose to use a more comprehensive name, i.e., "amplitude". Similar reasoning applies to the next one.
| * @brief Enum of the pulse parameter properties that can be queried via @ref | ||
| * QDMI_device_session_query_pulse_parameter_property as part of the @ref | ||
| * device_interface "device interface" and via @ref | ||
| * QDMI_device_query_pulse_parameter_property as part of the @ref | ||
| * client_interface "client interface". |
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 hope we did not vialoate this in the rest of QDMI, but a @brief description is a one-liner. You can move additional information into @details.
| /** | ||
| * @brief `bool` Whether the pulse parameter is mutable. | ||
| * @details If this property is set to true, the pulse parameter can be | ||
| * modified by the user. If it is set to false, the pulse parameter is fixed | ||
| * and cannot be changed. | ||
| */ | ||
| QDMI_PULSE_PARAMETER_PROPERTY_MUTABLE = 3, |
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 do not understand the purpose of immutable parameters, those are constants. I suggest that constants can already occur as constants in the formula.
| * @brief Enum of the pulse waveform properties that can be queried via @ref | ||
| * QDMI_device_session_query_pulse_waveform_property as part of the @ref | ||
| * device_interface "device interface" and via @ref | ||
| * QDMI_device_query_pulse_waveform_property as part of the @ref | ||
| * client_interface "client interface". |
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.
Same as above, should be a one-liner.
| QDMI_PULSE_WAVEFORM_PROPERTY_NAME = 0, | ||
| /** | ||
| * @brief `char*` (string) The formula used to generate the pulse waveform. | ||
| */ | ||
| QDMI_PULSE_WAVEFORM_PROPERTY_FORMULA = 1, |
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.
Ok, this will be a bigger comment. I'll put that in my general comment such that we can discuss and consolidate this in the main thread of this PR.
| QDMI_PULSE_IMPLEMENTATION_PROPERTY_OPENPULSE = 0, | ||
| /** | ||
| * @brief `QDMI_Pulse_Waveform` (@ref QDMI_Pulse_Waveform) The pulse | ||
| * implementation as pulse waveform. |
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.
| * implementation as pulse waveform. | |
| * implementation as a pulse waveform. |
?
|
Thank you @ystade for the detailed review and for moving the discussion forward. The current implementation is based on our initial discussion and is not final, but is intended to facilitate the discussion. I will address the inline comments once we have agreed on the basics. I am also not an expert on pulses, which is why I tagged @donsano33 and @amitQC. Hopefully, they will add their review soon. The pulse representation here is supposed to serve two purposes: one is to query the generic pulse shapes supported by the device, along with the constraints and pulses for specific operations. The second purpose is to set or overwrite existing pulses for a specific operation.
The OpenPulse Grammar can be used to query and set pulses specific to an operation, but as far as I know, it cannot be used to query generic pulse waveforms, i.e. the pulse formula and the limits of the parameters in that formula.
As far as I understand, the queried generic pulse waveform is to be consumed manually by a human, not by a compiler. The only automation tool I can think of at the moment would be one that consumes this information and generates human-readable documentation on the pulses supported by a device, along with the parameters and the limits on those parameters. I searched for a standard notation that could be specified in the QDMI specification, but I could not find one. I am thinking of proposing infix notation, which could be used by a tool such as str2sym from MATLAB. If you have any other ideas, please share them. |
|
Thanks @mnfarooqi and @ystade for starting the conversation. The idea of From the compiler's perspective, in my opinion, the only thing that matters is the corresponding operation to a pulse. For example, if a NOT gate has a sine waveform, you can take the performance of this pulse into account in FoMaC, but not the underlying pulse itself. |
|
Would you mind explaining the following point in your answer a bit further? I do not think that I understand this correctly. What information exactly matters to a compiler? How do I get from a random (I only write random because right now the string is not further specified and could in principle be anything) string to the performance?
|
Sure! Depending on the level of compilation, for a top-level, the underlying pulse implementing a gate |
Does that mean that the compiler will not use the pulse shape formula (i.e. the mathematical description of these waveforms or envelopes) directly? If not, where/who can use it? |
If compilers use the mathematical formula, it might be a very complex process (as the number of gates and qubits increases), because you have to deal with some complex parameters, compared to an object In my opinion (and as we also briefly discussed during our meetings), one can design pulses corresponding to beyond what the hardware offers, i.e., native gate sets using the formulas mentioned above or by giving a full complex array of amplitudes. If these are different pulse implementations corresponding to a gate |
|
Moving this thread forward, do either of you have any major comments or suggestions before I address the detailed comments on the code, @burgholzer and @ystade? |
I think the most important high level comment is still the one from Yannick (#182 (review)) |
|
I am moving this to the 1.3.0 milestone for now as there still seems to be ongoing discussions that need to be resolved and the changes on |
Description
Introduce opaque pointers (
QDMI_Pulse_Parameter,QDMI_Pulse_WaveformandQDMI_Pulse_Implementation), as well as the corresponding enums and query functions, to represent and query pulses.Fixes #173
Checklist: