-
Notifications
You must be signed in to change notification settings - Fork 127
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
Rework experiment options #39
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach makes sense to me. It is a bit hard to see what the default options are. Is there a way we can make it easier for a user to figure out what the defaults are?
qiskit_experiments/base_analysis.py
Outdated
"""Initialize a base analysis class | ||
|
||
Args: | ||
options: kwarg options for analysis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some more details on how the options are handled in the docstrings? Perhaps doing this at the module level instead of in the init or class docstring would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with having more description would be great, but it seems difficult. The Options
class defines nothing.
https://github.com/Qiskit/qiskit-terra/blob/9fd5436ba30b98d3bfd2578cc7f4ce564dfffd55/qiskit/providers/options.py#L18-L27
This options is provider dependent, but the experiment is provider agnostic (since this is analysis option, this can be experiment dependent).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, sounds good. I think we should probably add documentation of all experiment and analysis options to the class level docstring of each experiment and analysis options to the analysis class (this will duplicate analysis options, but I think this is good to do since most people will only interact via the Experiment class and not use the analysis classes directly).
@nkanazawa1989 The Options
class isn't provider dependent, its just a container in the qiskit provider module since it is currently only used in backends for IBMQ and Aer. Aer and IBMQ add their backend dependent options in the same way as here using _default_options
class methods.
if "initial_layout" in fields: | ||
raise QiskitError( | ||
"Initial layout cannot be specified as a transpile option" | ||
" as it is determined by the experiment physical qubits." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Are there any other such cases we need to worry about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I can immediately think of for circuits, maybe for calibration you will have some scheduling specific ones that should not be user changed though? If so we can add them as needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This almost looks like something that the subclasses would like to control. E.g. perhaps some RB experiments would want to run on a high optimization level while calibration experiments would want to avoid this. I suppose subclasses can always override set_transpile_options
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like we can override _default_transpile_options
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR makes the base class cleaner and I really like it. My main point is suggestion of removal of kwargs from Experiment.run method (though I understand this is convenient for single line execution, which is important concept of experiment).
@@ -59,8 +92,14 @@ def run(self, experiment_data, save=True, return_figures=False, **options): | |||
# Wait for experiment job to finish | |||
# experiment_data.block_for_result() | |||
|
|||
# Get runtime analysis options | |||
analysis_options = copy.copy(self.options) | |||
analysis_options.update_options(**options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The options
contains both immutable and runtime options? This overrides class default setting in constructor and runtime. For me it looks too flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was matching the current way options are handled for simulators. When a class is initialized it initializes a copy of the default options specified in the class design, and then everything in options is mutable. For analysis options it's currently coded so you can pass option values at construction Analysis(**options)
which is equivalent to setting them after default construction, Analysis().set_options(**options)
. These are equivalent. The Analysis.run
method can also be called with options, which will override any stored values in analysis.options
just for that execution, without changing any of the stored options.
However thinking about this more, since analysis options are also stored in experiment maybe this is overkill and we could remove the set_options
ahd just have the default options and run(**options)
to override them to keep the analysis clas more like a function without side effects (like it was before). I'll try make some changes to simplify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thank you :)
qiskit_experiments/base_analysis.py
Outdated
"""Initialize a base analysis class | ||
|
||
Args: | ||
options: kwarg options for analysis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with having more description would be great, but it seems difficult. The Options
class defines nothing.
https://github.com/Qiskit/qiskit-terra/blob/9fd5436ba30b98d3bfd2578cc7f4ce564dfffd55/qiskit/providers/options.py#L18-L27
This options is provider dependent, but the experiment is provider agnostic (since this is analysis option, this can be experiment dependent).
qiskit_experiments/base_analysis.py
Outdated
The options of an analysis class are used to provide kwarg values for | ||
the :meth:`run` method. | ||
""" | ||
return self._options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it is possible to have default data processor? I think this should depend on backend (execution) options, i.e. meas_level and return_type. Where the default data processor should be defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is probably a few ways to do this. One option is in the analysis class, see e.g. the work in progress here: #31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably makes sense to put something into the ExperimentData class for meas level. In ExperimentData when you add job/results and it pulls out circuit data it could for example store the meas_level / result_type in the metadata (if we didn't already add it as experiment metadata).
backend: Backend, | ||
analysis: bool = True, | ||
experiment_data: Optional[ExperimentData] = None, | ||
**kwargs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bit vague. **backend_options
? I think this run
method internally calls transpile, execute, and analysis. This makes me think this option should be able to handle all of these options. Otherwise we should remove runtime option to avoid confusion. i.e.
exp.run(backend, optimization_level=0)
doesn't look strange to me (since Options
class defines nothing we cannot raise any warning or error with current implementation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent is options passed to exeriment.run
are directly passed through to backend.run
as run options (and backend.run
doesnt support transpiler options like old execute
)
You can't filter out transpiler (or analysis) options without keeping an exhaustive list of all allowed option values for the transpiler like we had before, which is part of what this PR is trying to fix. So the intent is any transpiler/analysis options be specifically set using the corresponding set methods.
Another comment. Do we really need to expose transpile to end users? This sounds like a jargon. I prefer something like |
One thing here to note is I'm trying to make the |
@mtreinish not being able to add fields after construction will have some issues with transpiler/run/analysis options which do not provide an exhaustive list of all available fields in the default method (we don't want to duplicate every kwarg and default value from |
You can always create a new Options object if you need to add fields dynamically. I'm hesitant to enable adding fields after initialization because at a certain point it just ends up being a dictionary and I think the Options class should have more of a fixed structure so people can have more guarantees than a dictionary. For multiple backends being wrapped by an experiment class you can do something like what I did for the fake backends and just return the upstream backend's options object: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/test/mock/fake_backend.py#L112-L116 For the |
Thanks @chriseclectic, it looks good. Will it solve #17 ? |
@yaelbh It should, if you set the analysis options for each sub experiment. Something like: exp1 = Experiment(**params1)
exp1.set_analysis_otions(**opts1)
exp2 = Experiment(**params2)
exp2.set_analysis_options(**opts2)
exp12 = ParallelExperiment([exp1, exp2])
expdata = exp12.run(backend) |
Currently in the main branch all run-time options get passed to the analysis. Therefore, any circuit generation options that the analysis does not need also get passed to the analysis. To avoid the analysis from throwing an exception we need a |
@eggerdj Yes, that it one of the issues this PR addresses. To set analysis options now you could do exp = Experiment(...)
exp.set_analysis_options(**analysis_opts)
expdata = exp.run(backend, **run_opts) or if you wanted to break execution and analysis into 2 separate steps: exp = Experiment(...)
expdata = exp.run(backend, analysis=False, **run_opts)
exp.run_analysis(expdata, **analysis_options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. A few small questions/comments.
|
||
# Return the ExperimentData future | ||
return experiment_data | ||
|
||
def run_analysis(self, experiment_data, **options) -> ExperimentData: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few lines above is written:
self.run_analysis(experiment_data)
i.e. I see no options. Is this not inconsistent with the method signature? Do we need **options
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The direct call to run_analysis
allows overriding the experiments stored analysis options with the kwarg ones (like run does with runtime options). At the moment for split execution/analysis the following are basically equivalent:
# 1
exp.set_analysis_options(**analysis_opts)
expdata = exp.run(backend, analysis=False)
exp.run_analysis(expdata)
# 2
expdata = exp.run(backend, analysis=False)
exp.run_analysis(expdata, **analysis_opts)
Removing the kwargs from run_analysis would remove option 2. Opt 2 is not strictly necessary, but I thought it might be convenient in cases where you are running analysis separately to the main execution, or analyze expdata loaded from a DB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Thanks. At the risk of being pedantic, can we document this in the docstring to make the two usage options clear and that an analysis can be run outside the context of an experiment?
if "initial_layout" in fields: | ||
raise QiskitError( | ||
"Initial layout cannot be specified as a transpile option" | ||
" as it is determined by the experiment physical qubits." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This almost looks like something that the subclasses would like to control. E.g. perhaps some RB experiments would want to run on a high optimization level while calibration experiments would want to avoid this. I suppose subclasses can always override set_transpile_options
.
if "initial_layout" in fields: | ||
raise QiskitError( | ||
"Initial layout cannot be specified as a transpile option" | ||
" as it is determined by the experiment physical qubits." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like we can override _default_transpile_options
instead.
experiment_data: Optional, add results to existing | ||
experiment data. If None a new ExperimentData object will be | ||
returned. | ||
run_options: backend runtime options used for circuit execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it is worth adding a link to the API docs? Of course this is provider agnostic but user cannot find what is run_options with this. You can say something like this is provider dependent but in IBM Quantum you can refer to ...
* Reworks how experiment, transpiler, backend/run, and analysis options are handled in the base classes to make it similar to how options are used by qiskit backends. * Removes transpiled_circuits method. Transpiled circuits can now be obtained via `transpile(expr.circuits(backend), backend, **expr.transpile_options)`
* Remove `set_options` from `BaseAnalysis` * Add `run_analysis` method to `BaseExperiment` that uses experiment `analysis_options`. * Rename `backend_options` to `run_options`
* Rename Experiment.options to Experiment.experiment_options. * Add some more comments
…aProcessor as this will often be reused. * Aligned with PR qiskit-community#39 on options.
Summary
Currently it is difficult to set custom options for an experiment. This is in part due to the different layers of options that are involved including:
This PR attempts to improve how options are handled for both experiment design (for documenting available options and setting default values), and for the user (for customizing options when running experiments). This is done by adding set options methods in the same way that it is currently done for configurable Qiskit backends (Aer, IBMQ).
Note that this uses the same
Options
class as backends from terra, which currently is not mappable (so you need to do**options.__dict__
for example), however there is currently a PR open to make this class mappable.Details and comments
The main changes here are:
options
,transpile_options
,backend_options
, andanalysis_options
properties to the BaseExperiment class. These contain all default/set options for the current experiment and will be used by therun
method to allow customizing execution. When designing an experiment default values for each of these are set by overriding the corresponding_default_options
methods. For users to update options they use the correspondingset_options
,set_transpiler_options
etc methods.BaseExperiments.circuits
, these are intended to all be handled withoptions/set_options
.**options
toBaseExperiment.__init__
for setting custom option values at initialization.transpiled_circuits
method: this method should intead be done using a call to the terratranspile
function with the experimentstranspile_options
asqiskit.transpile(expr.circuits(backend), backend, **exp.transpile_options)
.With these changes an example of using custom options for running an experiment might look something like: