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

Better qubit and measurement lo interface #2832

Closed
taalexander opened this issue Jul 20, 2019 · 15 comments
Closed

Better qubit and measurement lo interface #2832

taalexander opened this issue Jul 20, 2019 · 15 comments
Assignees
Labels
mod: pulse Related to the Pulse module type: discussion type: enhancement It's working, but needs polishing

Comments

@taalexander
Copy link
Contributor

What is the expected enhancement?

I find the current lo interface confusing to use. I think it is a combination of naming and functionality. My main complaint is that:

  • qubit_lo_freq and meas_lo_freq seem to serve the same purpose as schedule_los

This can be confusing for users. I think it may be better to replace
qubit_lo_freq and meas_lo_freq by qubit_freq_est and meas_freq_est and then have the default behaviour to be too use these to create and pad schedule_los if not supplied. This way there is only one explicit behaviour to set the los for assembly (schedule_los).

Thoughts @nkanazawa1989 and @itoko?

@nkanazawa1989
Copy link
Contributor

nkanazawa1989 commented Jul 22, 2019

I remember why we didn't take the approach of

to replace qubit_lo_freq and meas_lo_freq by qubit_freq_est and meas_freq_est

is separation of schedule_los from backend and immutability of schedule_los , i.e. schedule_los should receive backend in its constructor to know qubit_freq_est and meas_freq_est . Currently everything about los is done in the assembler (because the assembler gets backend), but we don't need to output los in qobj when los are not supplied - default values will be automatically assigned in the backend. Please let me know if I don't answer to the question.

My another concern is naming issue (pulse.configuration.LoConfig), because #2650 will add another configuration for scheduler. We will have many schedule configuration s defined in different places and in different context. This is also confusing to use. I think these configurations should be combined.

@taalexander
Copy link
Contributor Author

The issue is naming. Right now it appears there are three ways to set the schedule lo qubit_lo_freq, meas_lo_freq and schedule_lo_freq. I understand the reason for having all three, but we should make it clearer that the qubit_lo_freq is really the frequencies of the qubits that the LO will default too.

My another concern is naming issue (pulse.configuration.LoConfig), because #2650 will add another configuration for scheduler. We will have many schedule configuration s defined in different places and in different context. This is also confusing to use. I think these configurations should be combined.

I don't really see the need for the users to ever use this class. They should be able to just provided a dictionary of channel:frequencies. Perhaps we could hide it?

@nkanazawa1989
Copy link
Contributor

qubit_lo_freq and meas_lo_freq are appeared only in qobj, so I think they are hidden from users unless the user directly assemble qobj. But I agree

the qubit_lo_freq is really the frequencies of the qubits that the LO will default

The only reason to have LoConfig class is self validation of frequency range. If we create pulse job validator, we don't need the class.

@taalexander
Copy link
Contributor Author

I don't think the above addresses my concerns. qubit_lo_freq and meas_lo_freq are arguments to assemble in conjunction with schedule_los this is confusing as these are really the qubit and measurement frequencies that as a matter of protocol we default to if schedule_los are not provided. Perhaps these should be default_qubit/meas_lo_freq?

@nkanazawa1989
Copy link
Contributor

nkanazawa1989 commented Jul 26, 2019

I understand your point. I agree. We need to simplify the configuration of los. qubit_lo_freq and meas_lo_freq can be used to set los which is common in all experiments, while schedule_los is used to set los depending on experiment.

As far as I understand,

  • (1) nothing
    this will set backend default qubit_lo_freq and meas_lo_freq as common configuration.

  • (2) qubit_lo_freq and meas_lo_freq, without schedule_los
    this will set custom qubit_lo_freq and meas_lo_freq as common configuration.

  • (3) single schedule_los, without qubit_lo_freq and meas_lo_freq
    this will set custom schedule_los as common configuration - just same as (2).

  • (4) n schedule_los of the same frequencies, without qubit_lo_freq and meas_lo_freq
    this will set qubit_lo_freq and meas_lo_freq for each experiment configuration, but this will give the same configuration as above (just increasing the size of qobj).

  • (5) qubit_lo_freq and meas_lo_freq, with n schedule_los
    this will overwrite backend default qubit_lo_freq and meas_lo_freq, and overwrite them by schedule_los in each experiment configuration.

now we have too many options to set lo configuration, and we should simplify this. As (3) can cover (2), we might be able to deprecate qubit_lo_freq and meas_lo_freq from arguments.

And I don't like the name qubit_lo_freq because this can be intentionally detuned from qubit frequency and "qubit" doesn't have a local oscillator (LO is bound to AWG). drive_freq and meas_freq sounds more natural to me.

@taalexander
Copy link
Contributor Author

Yes, I agree. The other subtlety is that if we only allow schedule_los the user must provide an lo for each qubit/measure channel. This is what I was thinking of when I was suggesting keeping qubit_freq_est and meas_freq_est as these are what the backend is going to use create the default lo. Do you think this is an issue?

@lcapelluto
Copy link
Contributor

lcapelluto commented Jul 26, 2019

I agree that drive_freq and meas_freq are the most accurate, we should support this at least in the user API. I also find schedule_lo confusing, and have been thinking about this problem. My first guess is we could do everything we wanted by just including drive and meas frequencies. What does schedule_los do that cannot be done with the others?

@nkanazawa1989 thank you for laying out a detailed map of all the current possibilities :)

@taalexander
Copy link
Contributor Author

What does schedule_los do that cannot be done with the others?

Schedule_lo right now is essentially a dictionary that is a mapping of {channel: frequency}. schedule_los can be a list of these. Right now if you do not specify all Drive and Measure channels, it will default qubit_lo_freq or meas_lo_freq which are typically set to qubit_freq_est and meas_freq_est (this is what is done if just a backend is provided).

@nkanazawa1989
Copy link
Contributor

nkanazawa1989 commented Jul 29, 2019

We are also able to complement schedule_los using the backend defaults inside assembler. In this case we don't need qubit_lo_freq and meas_lo_freq in arguments. We can specify a single schedule_los to overwrite backend default. On the other hand, I also feel it is reasonable to rename them to have schedule_los and default_los in the arguments.

I think we have three approaches (examples are how to overwrite default qubit drive[1] and apply frequency sweep of qubit drive[0]).

1. Rename qubit_lo_freq and meas_lo_freq to default_*

default_los = backend.defaults().qubit_freq_est
custom_los = default_los.copy()
custom_los[1] = 5.1

schedule_los = []
for drive_lo in drive_los:
    schedule_los.append({system.drives[0]: drive_lo})

qobj = assemble(schedule, backend, default_qubit_lo_freq=custom_los, schedule_los=schedule_los)

Pros:

  • Default lo frequencies are clear to users.
  • Minimum workload.

Cons:

  • Different formats for lo configuration - default los are List[float], while schedule_los is List[Dict[PulseChannels, float]].
  • Need to specify default frequencies for all channels.

2. Remove qubit_lo_freq and meas_lo_freq from arguments and replace by qubit_freq_est and meas_freq_est.

schedule_los = []
for drive_lo in drive_los:
    schedule_los.append({system.drives[0]: drive_lo, system.drives[1]: 5.1})

qobj = assemble(schedule, backend, schedule_los=schedule_los)

Pros:

  • Interface for lo configuration is unified.

Cons:

  • Default lo frequencies are not clear to users.
  • No explicit way to set default los (giving a single schedule_los will implicitly set default).

3. User LoConfig class for both default and schedule dependent

default_los = {system.drives[1]: 5.1}

schedule_los = []
for drive_lo in drive_los:
    schedule_los.append({system.drives[0]: drive_lo})

qobj = assemble(schedule, backend, default_los=default_los, schedule_los=schedule_los)

Pros:

  • Default frequencies are clear to users.
  • Same data format for default and schedule_los (interface becomes clean).

Cons:

  • Large workload.

@nkanazawa1989
Copy link
Contributor

nkanazawa1989 commented Jul 29, 2019

@lcapelluto I agree that schedule_los is bit confusing, but this is sometime very useful. e.g. when we do frequency sweep of qubit drive, only schedule_los enables us to combine all frequency sweeps in a single job. This is extremely useful to minimize a drift. Currently schedule_los consists of a list of LoConfig (when we provide los with python dictionary, LoConfig instance is generated internally). The reason we have this class is (1) to validate frequency range, and (2) to guarantee immutability. I think these are not mandatory and we can replace the class by python dictionary + validator to avoid confusing with ScheduleConfig of scheduler.

@taalexander
Copy link
Contributor Author

In general, this problem would be overcome if for each pulse channel we could specify the frame and sideband with which it should be applied. Then we could track these frames and sidebands separately (as we do already internally).

mtreinish pushed a commit that referenced this issue Aug 13, 2019
This PR adds more informative errors of qubit_lo_freq and meas_lo_freq are missing. A set of tests are added other assemble_schedule optional kwargs to ensure the desired behaviour.

As a follow-up #2832 has been raised to discuss a better lo interface.

Closes #2547

* Add tests for kwargs in assemble schedules. Make qubit_lo_freq and meas_lo_freq raise errors if not supplied.

* Error is now raised for missing qubit and measurement los.

* Added additional missing assemble kwarg tests.

* Change test assertion.

* Update changelog.

* linting.

* Fix lo range checking in lo_config.

* update memory slot assembly test.

* Fix merge bug.
@taalexander
Copy link
Contributor Author

Related to #3467. I'm not sure if the PR #3468 addresses the problem of frame. @eggerdj, this conversation may be interesting to you.

@eggerdj
Copy link
Contributor

eggerdj commented Nov 18, 2019

PR #3468 does not address the problem of frame.

After reading this conversion here is what I think would make sense from a physics point of view. Let's assume that the aim of PulseChannel is to generate Re[exp(i 2pi f j dt + phase) * d_j]. Then,

  1. Drive channel should default to the estimate of the qubit frequency called qubit_freq_est.
  2. Measure channel should default to the estimate of the readout resonator's frequency meas_freq_est.
  3. At assemble time, the user can override the default frequencies. This should be done with a dict data structure to avoid having to specify the frequency of the channels we do not wish to change. I would almost be tempted to call this dictionary initial_channel_frequencies.

Consequences:

  • For 1. and 2. I don't believe that we need to give assemble an input variable with the default frequencies. It seems that defaulting to qubit and resonator frequencies is the natural thing to do.
  • Point 3. implies that all schedules in a job have the same initial frequency.
  • Point 3. has some interesting consequences for validation, especially when the user can change f through SetChannelFrequency. From a validation perspective, and in the context of IQ mixing, the valid range for initial_channel_frequencies would be determined by the output range of the RF source (plus/minus sideband if one really wants to be nitpicky). Then any change to the frequency f (made through SetChannelFrequency) would have to be limited to the initial channel frequency, say f0, plus/minus the bandwidth of the AWG. This methodology of setting and initializing frequencies and changing them using SetChannelFrequency has the advantage of reflecting what is happening in the hardware. It allows you to accomodate setups with IQ mixing as well as setups with direct signal generation (a la Tek70k).

@taalexander
Copy link
Contributor Author

The one aspect I am still unclear on is how do we properly handle frames when changing frequencies. Consider the situation below for a cross-resonance gate with control i and target j.

  • For coupled qubits i, j we currently expose a drive channel for each and then a control channel coupling the two
  • In our physical hardware the control channel is actual the drive channel for i
  • Instead if we expose just the drivechannels to properly implement the CR gate we must be able to change the frequency and phase of drivechannel i to that of drivechannel j.

I don't think this is captured within the current SetFrequency instruction. It may make more sense to define a frame object, which has a frequency and a frame that evolves. Channels could then be assigned a frame.

@lcapelluto
Copy link
Contributor

We can close this. The SetFrequency command will allow us to deprecate these assemble arguments altogether. (#3468)

faisaldebouni pushed a commit to faisaldebouni/qiskit-terra that referenced this issue Aug 5, 2020
…#2833)

This PR adds more informative errors of qubit_lo_freq and meas_lo_freq are missing. A set of tests are added other assemble_schedule optional kwargs to ensure the desired behaviour.

As a follow-up Qiskit#2832 has been raised to discuss a better lo interface.

Closes Qiskit#2547

* Add tests for kwargs in assemble schedules. Make qubit_lo_freq and meas_lo_freq raise errors if not supplied.

* Error is now raised for missing qubit and measurement los.

* Added additional missing assemble kwarg tests.

* Change test assertion.

* Update changelog.

* linting.

* Fix lo range checking in lo_config.

* update memory slot assembly test.

* Fix merge bug.
@1ucian0 1ucian0 added the mod: pulse Related to the Pulse module label Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod: pulse Related to the Pulse module type: discussion type: enhancement It's working, but needs polishing
Projects
None yet
Development

No branches or pull requests

6 participants