Separate Qobj into PulseQobj and QASM Qobj

[nkanazawa1989]

Summary

This addresses the issues #1927 and #1928. This also adds unittest generating PulseQobj comprising a basic pulse sequence. Generated PulseQobj through marshmallow can be passed to the backend.

I renamed all Qobj ( QobjConfig ) to QASMQobj ( QASMQobjConfig ). However, qiskit.provider also imports qiskit.qobj.Qobj and qiskit.qobj.QobjConfig . We also need to change the names otherwise from qiskit import IBMQ causes import error.

Details and comments

I created BaseQobjConfig . PulseQobjConfig and QASMQobjConfig inherit BaseQobjConfig . This is because PulseQobj and QASMQobj have some common elements (e.g. shots , memory_slots , etc...)

[taalexander]

This looks great Naoki! The only change that I request is that the Pulse and QASM object varieties inherit from a common parent. This will make it easier to do type checking for parts of the stack that don't care about pulse or qasm, such as job submission.

[nkanazawa1989]

@taalexander Thank you for your suggestion. I changed all structures to inherit Qobj as a superclass. Now from qiskit import IBMQ works.

[diego-plan9]

Thanks @nkanazawa1989 ! The review turned out to be a rather verbose one, but most of the inline comments are quite small one-liners and aimed at being a bit exhaustive in order to have the models as fleshed out as possible - which I think they are! 👍

I also have some changes that I'm including in this comment, to separate them a bit from the inline comments - as they are more architectural/organizative, and actually higher priority overall.

• can we keep QobjHeader and QobjExperimentHeader as the only models used for headers (ie. remove QASMQobjHeader, PulseQobjExperimentHeader, etc)? Unless the specs have changed, the headers overall were meant to be "generic and loosely defined (as in, they have no standard properties) objects that are passed back and forth"- making subclassing not provide too much value, since they have the same semantic meaning and structure in all kinds of Qobj.

• can we revise QobjExperiment, adding attributes to it? At the moment the schema contains no properties, but actually an experiment seems to always have instructions (required), header and config regardless of the type. Even if each attribute is slightly specialized by the subclasses (except for header as per the comment above), it would make the structure more generic and provide stronger assurances when using the base class.

• on the organizative side:

  • can we drop Base from the generic-qobj schemas (ie. BaseQobjConfigSchema -> QobjConfigSchema? Currently is a slight deviation from the previous convention (FooSchema defines Foo), and it is not providing too much value to have it in the names, which are getting long enough already!
  • can we also change QASMFoo to QasmFoo? I'm not strongly set on this one, but we have some other similarly named classes where we opted for not fully capitalizing (QasmSimulatorPy, QasmError), and feels slightly more homogeneous.

• also on the organizative side: can we move the classes in models.py to smaller, more specific modules? Since each entity requires two classes, the number of classes in the file (42) is getting a bit above the recommended threshold for mantainability - splitting them would make it easier to focus on the classes needed for each purpose, and allow better comparison between them for developers. My suggestion would be:

  qiskit/qobj/qasm.py -> the qasm-specific items (`QasmQobjConfig`, etc)
  qiskit/qobj/pulse.py -> the pulse-specific items (`PulseQobjConfig`, etc)
  qiskit/qobj/models.py -> the generic qobj items (`QobjConfig`, etc) - maybe a more generic name for the file, such as `base.py` is in order, but I'm not too sure
  

[nkanazawa1989]

Thank you @diego-plan9 ! I reflected all of your comments in the latest commits.

• Header schemas are integrated into the generic schema.
• QobjExperimentSchema is updated to have instruction.

• class name changes, separation of qobj models, and some minor changes.

I need to add another schema for a pulse simulator, but this will be a separate PR.

[nkanazawa1989]

I merged latest master branch in wrong way and corrected this in 11f1b6f. Sorry about changing commit hash by my mistake.

[diego-plan9]

Thanks @nkanazawa1989 - it ended up being a nice PR, specially considering dealing with the schemas and everything related! As mentioned in other conversations, MeasurementParameter has not been fully reviewed during this PR, and should be reviewed along with the depending PRs that use it. Good work!