Skip to content
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

Pulse amplitude representation in pulse library #6097

Closed
nkanazawa1989 opened this issue Jan 31, 2020 · 6 comments · Fixed by #9002
Closed

Pulse amplitude representation in pulse library #6097

nkanazawa1989 opened this issue Jan 31, 2020 · 6 comments · Fixed by #9002

Comments

@nkanazawa1989
Copy link
Contributor

What is the expected behavior?

From a calibration point of view, pulse amplitude is better to be represented in (float |amp|, float phase) rather than complex amp. Usually I tune pulse amplitude by amplitude sweep followed by error amplification, and then calibrate the phase separately. I often write wrapper function of one in the pulse library to convert (amp, phase) into complex value as an input, but changing the argument of those functions is more straightforward. Since they are public function, is it worth changing the interface?

@taalexander
Copy link
Contributor

taalexander commented Feb 3, 2020

I don't think what you are doing is standard, typically pulses are represented as a complex parameter A for the amplitude scaling (see the original DRAG paper for example) and waveforms are taken to be complex as they linearly scale Hamiltonian terms which are themselves made up of Hermitian matrices and these are complex square matrices. Separating the amplitude and the phase would obfuscate this correlation.

@nkanazawa1989
Copy link
Contributor Author

Good point. As you said we need complex value representation for DRAG. However, as written in the ref below, we calibrate phase and amplitude separately for gaussian square, ie CR, pulse. Since the phase controls rotation axis and the amplitude controls rotation angle, (amp, phase) representation is convenient for this type of calibration.

https://arxiv.org/abs/1603.04821

@nkanazawa1989
Copy link
Contributor Author

nkanazawa1989 commented Feb 4, 2020

I don't mean changing data representation (sorry about confusing wording), but I just propose to change the interface, or arguments, of pulse library functions and ParametricPulse. Of course it generates complex valued SamplePulse object or complex amplitude for ParametricPulse qobj regardless of my suggestion here.

This interface is also convenient for DRAG. For example, we can intuitively create both X and Y gates with (amp, phase) interface: phase = 0.5*np.pi for Y gates (if electrical length of DAC-mixer's IQ channel is the same).

If we want to keep the interfaces, maybe it is worth adding decorator that accepts (amp, phase) argument and convert it into complex to call original functions.

@taalexander
Copy link
Contributor

taalexander commented Feb 4, 2020

Another reason why we would not want to encourage this way of defining pulses is that it will result in people using twice as much memory on the device. For example, defining an X and the Y pulse in this way would be more efficiently accomplished with a pre and post frame change around X. I don't see what is so hard about doing amp*np.exp(-0.5j*np.pi).

@nkanazawa1989
Copy link
Contributor Author

The DRAG case is just an example and I agree with using FC is smart solution for phase control. Yes, I always do like amp*np.exp(-0.5j*np.pi) but writing this is little bit of pain. For example, if the interface supports phase, we can store cal parameters as a dict and pass it as **kwargs to create pulse. Currently I need to write new function to receive (amp, phase) to do it, or call pulse lib function and give params one by one with, eg duration=dict.get('duration'), amp=dict.get('amp')*np.exp(-1j*dict.get('phase)), sigma ....

Another trivial reason of using (amp, phase) is the spec of ParameterExpression object. Since it doesn't support complex data, we cannot convert parametrized schedule into circuit when the amplitude is parametrized.

This is not critical issue and just something nice to have. If no one agreed, please close the issue.

@taalexander
Copy link
Contributor

taalexander commented Feb 26, 2020

For example, if the interface supports phase, we can store cal parameters as a dict and pass it as **kwargs to create pulse

you can store a complex number for the amplitude.

Another trivial reason of using (amp, phase) is the spec of ParameterExpression object. Since it doesn't support complex data, we cannot convert a parametrized schedule into circuit when the amplitude is parametrized.

Fair point although I think this should support operations soon such as multiplication.

If you want to add a phase, I am ok with that as a kwarg, phase that by default is zero. Behind the scenes we will perform the multiplication for the parameterized pulse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants