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

Update the qobj schema to support pulse gate calibrations. #4761

Merged
merged 43 commits into from
Aug 21, 2020

Conversation

lcapelluto
Copy link
Contributor

@lcapelluto lcapelluto commented Jul 20, 2020

Summary

Begin Pulse Gates feature by updating the schema. Calibrations may be in the job config or the experiment config.

Example:

// job: QasmQobj
// job.config
{
  'pulse_library': [
    // same as pulse lib for PulseQobj
  ],
  'calibrations': {'gates': [
      // see below
    ],
  }
}

// job.experiments[0].config.calibrations
{
  'gates: [
    {"name": "rxtheta", "qubits": [0], "params": [3.14], "instructions": [openpulse_instructions]},
    {"name": "rxtheta", "qubits": [0], "params": [1.57], "instructions": [openpulse_instructions]},
    {"name": "rxtheta", "qubits": [1], "params": [1.57], "instructions": [openpulse_instructions]}
  ]
}

The 'gates' information must be sufficient to match against any QASM Qobj instruction within the job.experiments[i].instructions fields, for instance: QasmQobjInstruction(name='rxtheta', params=[1.57], qubits=[1]) matches the last entry.

We can move pulse_library to the top level to avoid duplicating memory-intensive pulse waveforms in a single job. The assembler will assert that each waveform in the pulse library is referenced uniquely.

Details and comments

  • TODO: add an example json
  • TODO: GateCalibration object
  • TODO: Move pulse_library to qobj level
  • TODO: add qobj.config.calibrations field
  • Update example json with some top level calibrations

Limitations

The current iteration supports any possible job, however, it is of course less efficient than a parameterized Qobj would be.

This schema cannot support unbound parameters, which is also true for any circuit at the moment, except here: Qiskit/qiskit-aer#485. Supporting unbound parameters should be done in a follow up (e.g., a gate's params are not fully specified and a value in the openpulse_instructions depends on that param). This follow up should be useable/compatible with Pulse Schedule, as well.

@lcapelluto lcapelluto marked this pull request as ready for review July 20, 2020 20:43
@lcapelluto lcapelluto requested a review from a team as a code owner July 20, 2020 20:43
qiskit/qobj/qasm_qobj.py Outdated Show resolved Hide resolved
qiskit/schemas/qobj_schema.json Show resolved Hide resolved
@taalexander taalexander added the type: enhancement It's working, but needs polishing label Jul 20, 2020
@taalexander taalexander self-assigned this Jul 20, 2020
qiskit/qobj/qasm_qobj.py Outdated Show resolved Hide resolved
qiskit/qobj/qasm_qobj.py Outdated Show resolved Hide resolved
@lcapelluto lcapelluto mentioned this pull request Aug 10, 2020
2 tasks
Copy link
Contributor

@taalexander taalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good except for needing top-level calibrations and my one comment regarding the lazy loading

qiskit/qobj/common.py Show resolved Hide resolved
Copy link
Member

@SooluThomas SooluThomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicks

qiskit/qobj/common.py Outdated Show resolved Hide resolved
qiskit/qobj/qasm_qobj.py Outdated Show resolved Hide resolved
@lcapelluto lcapelluto requested a review from SooluThomas August 18, 2020 16:11
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM except for one backwards compatibility concern around moving the validator module property. I know that's being used downstream by people and we shouldn't break them unless we have to. We either should alias and deprecate it with a release note or just leave it in qobj/qasm.py.

qiskit/qobj/common.py Show resolved Hide resolved
qiskit/qobj/qasm_qobj.py Show resolved Hide resolved
@lcapelluto lcapelluto requested a review from mtreinish August 19, 2020 13:56
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for checking the deepcopy/pickling

Copy link
Contributor

@taalexander taalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, all suggestions are non-blocking.

@@ -464,6 +476,19 @@ def __init__(self, qobj_id=None, config=None, experiments=None,
self.type = 'QASM'
self.schema_version = '1.3.0'

def _validate_json_schema(self, out_dict):
class QobjEncoder(json.JSONEncoder):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be broken out as a separate utility class?

],
"instructions": [
{
"name": "1c5a0346ff72c04c45d536d50d13b0d42cf65dc8e320d32385d36861d3bb1e41",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These hashed names are pretty cryptic for an example.

Comment on lines +412 to +413
"""
Initialize a single gate calibration. Instructions may reference waveforms which should be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""
Initialize a single gate calibration. Instructions may reference waveforms which should be
"""Initialize a single gate calibration. Instructions may reference waveforms which should be

"type": "object",
"properties": {
"gates": {
"description": "A list of dicts which contain all the required information to lower a gate to waveforms.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "A list of dicts which contain all the required information to lower a gate to waveforms.",
"description": "A list of objects which contain all the required information to lower a gate to waveforms.",

"type": "object",
"properties": {
"gates": {
"description": "A list of dicts which contain all the required information to lower a gate to waveforms.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "A list of dicts which contain all the required information to lower a gate to waveforms.",
"description": "A list of objects which contain all the required information to lower a gate to waveforms.",

@mergify mergify bot merged commit 68f65d2 into Qiskit:master Aug 21, 2020
@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: enhancement It's working, but needs polishing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants