Remove pulse defaults required parameters #2101
Conversation
taalexander
requested review from
1ucian0,
ajavadia,
atilag,
delapuente,
diego-plan9,
ewinston,
jaygambetta,
kdk,
mtreinish and
nonhermitian
as code owners
ajavadia
added
type: specs
| meas_kernel = Nested(MeasurementKernelSchema, required=True) | ||
| discriminator = Nested(DiscriminatorSchema, required=True) | ||
| qubit_freq_est = List(Number(), required=False, | ||
| validate=Length(min=1), missing=None) |
There was a problem hiding this comment.
As discussed in private, can we avoid having missing=, and instead either encourage the backends to provide sensible values for the attributes, or handle them somewhere in the pulse layers in terra? The rationale is that having a "default" blurs the separation between models (acting as "dummy containers") and logic a bit, and requires consumers to not only take into account the models, but the defaults - and if they reach that point, they are probably better off handling that in a different layer altogether, closer to the one that implements the functionality that uses those parameters or similar.
| buffer = Integer(required=False, validate=Range(min=0), missing=0) | ||
| pulse_library = Nested(PulseLibraryItemSchema, required=True, many=True, missing=lambda: []) | ||
| meas_kernel = Nested(MeasurementKernelSchema, required=False, missing=None) | ||
| discriminator = Nested(DiscriminatorSchema, required=False, missing=None) | ||
|
|
||
| # Optional properties. |
There was a problem hiding this comment.
For consistency with rest of the codebase, can you remove required=False from the optional parameters and place them under:
# Optional properties.

| @@ -140,13 +140,4 @@ class PulseDefaults(BaseModel): | |||
| discriminator (Discriminator): Default discriminator. | |||
| """ | |||
|
|
|||
| def __init__(self, qubit_freq_est, meas_freq_est, buffer, pulse_library, | |||
There was a problem hiding this comment.
Also for consistency with the rest of the codebase - if some of the parameters end up being required=True, can you make sure they are included in the constructor parameters and set inside it?
| @@ -140,13 +140,5 @@ | |||
| "type": "array" | |||
| } | |||
| }, | |||
| "required": [ | |||
There was a problem hiding this comment.
Also a reminder to make sure that if some of the parameters end up being required, they should be kept in this section of the .json, in the hopes of facilitating keeping them in sync eventually.
…-defaults-required
…iskit-terra into issue-2098-pulse-defaults-required
* Removed required default parameters. * Update changelog. Update defaults model validation to add nonetypes. * Update changelog. * Readd parameters to be required. * Update changelog and defaults required parameters.
Summary
Makes defaults parameters non-required. This is to not break the
defaultsendpoint for non-pulse backends and simulators.Details and comments
Makes all parameters non-required. For
cmd_defandpulse_librarydefault missing values are lists, for other parameters they are set toNone.