Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
c0dcd99
split run into transpile/execute/analysis and add hooks
Sep 1, 2021
5724afa
update composite experiment to retain hooks and configurations
Sep 7, 2021
c0cd0a7
black & lint
Sep 7, 2021
9bb746a
fix unittests
Sep 7, 2021
be9ce8c
add proper test
Sep 7, 2021
8492bc2
black & lint
Sep 7, 2021
f1a1bcb
Update qiskit_experiments/framework/base_experiment.py
nkanazawa1989 Sep 13, 2021
88f1304
update method signature and docs
Sep 13, 2021
4baca73
Merge branch 'feature/experiment_run_hook_simple' of github.com:nkana…
Sep 13, 2021
80a689e
rewrite complicated logic
Sep 13, 2021
3fa62e1
update docs
Sep 13, 2021
2abc6d8
update warnings for composite experiments
Sep 13, 2021
b7b1128
update description of test
Sep 13, 2021
ad0074f
fix fake backend
Sep 13, 2021
d5ca18d
Update qiskit_experiments/framework/base_experiment.py
nkanazawa1989 Sep 13, 2021
502da20
black&lint
Sep 13, 2021
8fca8e9
Merge branch 'feature/experiment_run_hook_simple' of github.com:nkana…
Sep 13, 2021
26c6dd4
move duplicated code to external function
Sep 14, 2021
c238504
docstring
Sep 14, 2021
4319d70
add composite transpile option test and reno
Sep 14, 2021
690d2ec
slightly update reno
Sep 14, 2021
d56612c
remove redundant error message with update
Sep 14, 2021
b968434
Merge branch 'main' of github.com:Qiskit/qiskit-experiments into feat…
Sep 14, 2021
64a6469
remove note comment to class documentation
Sep 14, 2021
003b9be
update docs
Sep 14, 2021
91c8de9
lint
Sep 14, 2021
e234afa
make hooks protected member
Sep 28, 2021
9250b6e
keep qubit ordering
Sep 28, 2021
f24f5cb
add unittest
Sep 28, 2021
bf34b77
revert change to run_analysis
Sep 28, 2021
5768915
Merge branch 'main' of github.com:Qiskit/qiskit-experiments into feat…
Sep 28, 2021
b7f8ac0
add None check
Sep 28, 2021
1df06b2
black&lint
Sep 28, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 101 additions & 43 deletions qiskit_experiments/framework/base_experiment.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
from qiskit.providers import BaseJob
from qiskit.providers.backend import Backend
from qiskit.providers.basebackend import BaseBackend as LegacyBackend
from qiskit.test.mock import FakeBackend
from qiskit.exceptions import QiskitError
from qiskit.qobj.utils import MeasLevel
from qiskit_experiments.framework import Options
Expand Down Expand Up @@ -96,10 +95,6 @@ def run(

Returns:
The experiment data object.

Raises:
QiskitError: if experiment is run with an incompatible existing
ExperimentData container.
"""
# Create experiment data container
experiment_data = self._initialize_experiment_data(backend, experiment_data)
Expand All @@ -109,43 +104,25 @@ def run(
run_opts.update_options(**run_options)
run_opts = run_opts.__dict__

# Scheduling parameters
if backend.configuration().simulator is False and isinstance(backend, FakeBackend) is False:
timing_constraints = getattr(self.transpile_options.__dict__, "timing_constraints", {})
timing_constraints["acquire_alignment"] = getattr(
timing_constraints, "acquire_alignment", 16
)
scheduling_method = getattr(
self.transpile_options.__dict__, "scheduling_method", "alap"
)
self.set_transpile_options(
timing_constraints=timing_constraints, scheduling_method=scheduling_method
)

# Generate and transpile circuits
transpile_opts = copy.copy(self.transpile_options.__dict__)
transpile_opts["initial_layout"] = list(self._physical_qubits)
circuits = transpile(self.circuits(backend), backend, **transpile_opts)
self._postprocess_transpiled_circuits(circuits, backend, **run_options)
circuits = self.run_transpile(backend)

# Execute experiment
if isinstance(backend, LegacyBackend):
qobj = assemble(circuits, backend=backend, **run_opts)
job = backend.run(qobj)
else:
job = backend.run(circuits, **run_opts)

# Add Job to ExperimentData and add analysis for post processing.
run_analysis = None

# Add experiment option metadata
self._add_job_metadata(experiment_data, job, **run_opts)

if analysis and self.__analysis_class__ is not None:
run_analysis = self.run_analysis

experiment_data.add_data(job, post_processing_callback=run_analysis)
# Run analysis
if analysis:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This block looks strange, that if you are doing analysis run_analysis handles adding job to data, but if you dont, this function handles adding data.

experiment_data = self.run_analysis(experiment_data, job)
else:
experiment_data.add_data(job)

# Return the ExperimentData future
return experiment_data

def _initialize_experiment_data(
Expand All @@ -167,29 +144,114 @@ def _initialize_experiment_data(

return experiment_data._copy_metadata()

def run_analysis(self, experiment_data, **options) -> ExperimentData:
def _pre_transpile_hook(self, backend: Backend):
Comment thread
yaelbh marked this conversation as resolved.
Outdated
"""An extra subroutine executed before transpilation.
Comment thread
nkanazawa1989 marked this conversation as resolved.

Args:
backend: Target backend.
"""
pass

# pylint: disable = unused-argument
def _post_transpile_hook(
self, circuits: List[QuantumCircuit], backend: Backend
) -> List[QuantumCircuit]:
"""An extra subroutine executed after transpilation.
Comment thread
eggerdj marked this conversation as resolved.

Args:
circuits: List of transpiled circuits.
backend: Target backend.

Returns:
List of circuits to execute.
"""
return circuits

def run_transpile(self, backend: Backend, **options) -> List[QuantumCircuit]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's better to have this interface instead:

 def run_transpile(self, circuits, backend: Backend, **options) -> List[QuantumCircuit]: 

That is to have the the circuits generation phase called explicitly in the run method and have its results passed to run_transpile. Better from single responsibility and modularity principle

@nkanazawa1989 nkanazawa1989 Sep 13, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This causes a problem in CompositeExperiment.run_transpile. We need to apply transpile options and pre/post method for each experiment, thus circuits here will be List[List[QuantumCircuits]] for CompositeExperiment while it will be List[QuantumCircuit] for BaseExperiment. This is why circuit generation is not done in the run method, because we cannot combine un-transpiled circuits as currently implemented in the composite experiments (current logic doesn't support mixture of different experiments).

Another advantage of this signature would be #380 (comment). User can easily check what will be executed. If we assume circuit is always generated by run method, the run_transpile should be a protected method.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

According to your comment, probably run_transpile is not correct name for this function. Something like run_circuit_generation would make more sense?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there any reason to have this function instead of just having a circuits function that returns the transpiled circuit. You could change the existing circuit function to be _circuits, and then have the public circuit function take a backend as argument and function as:

def _circuits(self, backend=None):
    # equivalent to existing circuit method for current experiments

def circuits(self, backend=None, transpile=True, **options):
     circuits = self._circuits(backend)
     if transpile:
         transpile_options = ...
         circuits = transpile(self._circuits(backend), backend, **transpile_options)
         self._post_transpile_action(circuits, backend)
     return circuits

Maybe you don't need the transpile kwarg and can just always transpile

@nkanazawa1989 nkanazawa1989 Sep 27, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I like this approach. However, this always returns transpiled circuit, i.e. even single qubit experiment returns full qubit circuits. Sometime this make it difficult to understand what is happening in the experiment.

"""Run transpile and returns transpiled circuits.
Comment thread
nkanazawa1989 marked this conversation as resolved.
Outdated

Args:
backend: Target backend.
options: User provided runtime options.

Returns:
Transpiled circuit to execute.
"""
# Run pre transpile. This is implemented by each experiment subclass.
Comment thread
nkanazawa1989 marked this conversation as resolved.
Outdated
self._pre_transpile_hook(backend)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think those calls should be at the run method level instead of within the run_transpile and run_analysis methods. This will keep the run_<transpile|analysis> methods more self contained and atomic, hence facilitating reuse and full customizability in the rare or advanced cases where the run method should be overriden and tweak the flow.
Furthermore, in a CompositeExperiment setting, do we always want the pre|post methods to be called for each invocation of a child experiment, or maybe only in certain cases or even not at all ? So I think that by moving the pre|post method calls to the run method we'll have full control of it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't assume any situation we need to customize run_transpile and run_analysis, and we should be able to control all experiments with pre and post methods. Having them in the run method, I found that we need to write almost the same run logic (but slightly different) for composite experiment, and I came up with integration into run_transpile / run_analysis (again, given these methods are not expected to be updated). If we allow child class to override them, indeed, we don't need to provide pre/post methods.

The another important point in my mind is

exp1 = Experiment1(**options1)
exp2 = Experiment2(**options2)

data1 = exp1.run(backend)
data2 = exp2.run(backend)

par_exp = ParallelExperiment(exp1, exp2)
par_data = par_exp.run(backend)

assert parallel_result.component_experiment_data(0) == data1 
assert parallel_result.component_experiment_data(1) == data2

This example means a composite experiment is just a macro that combines experiments for efficient execution, and it should NOT implement own pre/post methods not to override the experimental configuration of sub experiments. I think the composite experiment should always call pre/post methods of nested experiments.


# Get transpile options
transpile_options = copy.copy(self.transpile_options)
transpile_options.update_options(
initial_layout=list(self._physical_qubits),
**options,
)
transpile_options = transpile_options.__dict__

circuits = transpile(circuits=self.circuits(backend), backend=backend, **transpile_options)

# Run post transpile. This is implemented by each experiment subclass.
circuits = self._post_transpile_hook(circuits, backend)

return circuits

def _post_analysis_hook(self, experiment_data: ExperimentData):
Comment thread
eggerdj marked this conversation as resolved.
Outdated
"""An extra subroutine executed after analysis.

Args:
experiment_data: A future object of the experiment result.

Note:
The experiment_data may contain a future object as an experiment result
and the previous analysis routine has not completed yet.
If the hook should be executed immediately, call :meth:`block_for_results` method
before starting the data processing code.
"""
pass

def run_analysis(
self, experiment_data: ExperimentData, job: BaseJob = None, **options

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do you need to add the job to the arguments of this function?

@nkanazawa1989 nkanazawa1989 Sep 28, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think your PR #398 will improve this. I wanted to tie analysis to post analysis so that composite experiment can easily handle entire analysis logic. Otherwise composite needs to guarantee the matching of analysis and post analysis, and the logic will be bit more complicated.

) -> ExperimentData:
"""Run analysis and update ExperimentData with analysis result.

Args:
experiment_data (ExperimentData): the experiment data to analyze.
options: additional analysis options. Any values set here will
override the value from :meth:`analysis_options`
for the current run.
experiment_data: The experiment data to analyze.
job: The future object of experiment result which is currently running on the backend.
options: Additional analysis options. Any values set here will
override the value from :meth:`analysis_options` for the current run.

Returns:
An experiment data object containing the analysis results and figures.

Raises:
QiskitError: if experiment_data container is not valid for analysis.
QiskitError: Method is called with an empty experiment result.
"""
run_analysis = self.analysis() if self.__analysis_class__ else None

# Get analysis options
analysis_options = copy.copy(self.analysis_options)
analysis_options.update_options(**options)
analysis_options = analysis_options.__dict__

# Run analysis
analysis = self.analysis()
analysis.run(experiment_data, **analysis_options)
if not job and run_analysis is not None:
# Run analysis immediately
if not experiment_data.data():
raise QiskitError(
"Experiment data seems to be empty and no running job is provided. "
"At least one data entry is required to run analysis."
)
experiment_data = run_analysis.run(experiment_data, **analysis_options)
else:
# Run analysis when job is completed
experiment_data.add_data(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This gives this function a confusing signature, if i call it with data and a job it is going to try and add that job to the data, but adding job data to experiment data should probably always be done by the run method, not the run_analysis method.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is fixed in bf34b77

data=job,
post_processing_callback=run_analysis.run,
**analysis_options,
)

# Run post analysis. This is implemented by each experiment subclass.
self._post_analysis_hook(experiment_data)

return experiment_data

@property
Expand Down Expand Up @@ -335,10 +397,6 @@ def set_analysis_options(self, **fields):
"""
self._analysis_options.update_options(**fields)

def _postprocess_transpiled_circuits(self, circuits, backend, **run_options):
"""Additional post-processing of transpiled circuits before running on backend"""
pass

def _metadata(self) -> Dict[str, any]:
"""Return experiment metadata for ExperimentData.

Expand Down
92 changes: 55 additions & 37 deletions qiskit_experiments/framework/composite/batch_experiment.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,62 +13,80 @@
Batch Experiment class.
"""

from collections import OrderedDict
from typing import List

from qiskit import QuantumCircuit


from .composite_experiment import CompositeExperiment


class BatchExperiment(CompositeExperiment):
"""Batch experiment class"""
"""Batch experiment class.

This experiment takes multiple experiment instances and generates
a list of flattened circuit to execute.
Comment thread
nkanazawa1989 marked this conversation as resolved.
Outdated
The experimental circuits are executed ony be one on the target backend as a single job.

If an experiment analysis needs results of different types of experiments,
``BatchExperiment`` may be convenient to describe the flow of the entire experiment.

The experimental result of ``i``-th experiment can be accessed by

.. code-block:: python3

batch_exp = BatchExperiment([exp1, exp2, exp3])
batch_result = batch_exp.run(backend)

exp1_res = batch_result.component_experiment_data(0) # data of exp1
exp2_res = batch_result.component_experiment_data(1) # data of exp2
exp3_res = batch_result.component_experiment_data(2) # data of exp3

One can also create a custom analysis class that estimates some parameters by
combining above analysis results. Here the ``exp*_res`` is a single
:py:class:`~qiskit_experiments.framework.experiment_data.ExperimentData` class of
a standard experiment, and the associated analysis will be performed once the batch job
is completed. Thus analyzed parameter value of each experiment can be obtained as usual.

.. code-block:: python3

param_x = exp1_res.analysis_results("target_parameter_x")
param_y = exp2_res.analysis_results("target_parameter_y")
param_z = exp3_res.analysis_results("target_parameter_z")

param_xyz = param_x + param_y + param_z # do some computation

The final parameter ``param_xyz`` can be returned as an outcome of this batch experiment.
"""

def __init__(self, experiments):
"""Initialize a batch experiment.

Args:
experiments (List[BaseExperiment]): a list of experiments.
"""

# Generate qubit map
self._qubit_map = OrderedDict()
logical_qubit = 0
for expr in experiments:
for physical_qubit in expr.physical_qubits:
if physical_qubit not in self._qubit_map:
self._qubit_map[physical_qubit] = logical_qubit
logical_qubit += 1
qubits = tuple(self._qubit_map.keys())
qubits = sorted(set(sum([list(expr.physical_qubits) for expr in experiments], [])))
Comment thread
nkanazawa1989 marked this conversation as resolved.
Outdated
super().__init__(experiments, qubits)

def circuits(self, backend=None):
def _flatten_circuits(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not keeping the name circuits ? What if we'll have a new type of a composite experiment subclass in which we the circuit generation is not really flattening or concatenation ? Maybe some variational algorithm for example. I think circuits is a proper general term. Just a thought...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh this is due to poor gitdiff. The circuits method still exists but not used because we always get circuit from nested experiments.
https://github.com/Qiskit/qiskit-experiments/blob/d5ca18d207a6deb6ed690b6ebdc68c2ed8b5246b/qiskit_experiments/framework/composite/composite_experiment.py#L130-L143

Actually this is one of my concerns in this PR. Previously, this circuits method provides a logic to combine circuits without transpiling. However, this doesn't allow us to combine different types of experiments, e.g. RB and calibration, due to different transpilation behavior.

I think an inner-loop of a variational algorithm can be implemented as a BaseExperiment rather than composite. The loop type experiment should be different base class because these base classes don't implements a logic for iteration.

self,
circuits: List[List[QuantumCircuit]],
num_qubits: int,
) -> List[QuantumCircuit]:
"""Flatten circuits.

Note:
This experiment concatenates sub experiment circuits.
Comment thread
nkanazawa1989 marked this conversation as resolved.
Outdated
"""
batch_circuits = []

# Generate data for combination
for index, expr in enumerate(self._experiments):
if self.physical_qubits == expr.physical_qubits:
qubit_mapping = None
else:
qubit_mapping = [self._qubit_map[qubit] for qubit in expr.physical_qubits]
for circuit in expr.circuits(backend):
# Update metadata
circuit.metadata = {
for expr_idx, sub_circs in enumerate(circuits):
for sub_circ in sub_circs:
sub_circ.metadata = {
"experiment_type": self._type,
"composite_metadata": [circuit.metadata],
"composite_index": [index],
"composite_index": [expr_idx],
"composite_metadata": [sub_circ.metadata],
}
# Remap qubits if required
if qubit_mapping:
circuit = self._remap_qubits(circuit, qubit_mapping)
batch_circuits.append(circuit)
return batch_circuits
batch_circuits.append(sub_circ)

def _remap_qubits(self, circuit, qubit_mapping):
"""Remap qubits if physical qubit layout is different to batch layout"""
num_qubits = self.num_qubits
num_clbits = circuit.num_clbits
new_circuit = QuantumCircuit(num_qubits, num_clbits, name="batch_" + circuit.name)
new_circuit.metadata = circuit.metadata
new_circuit.append(circuit, qubit_mapping, list(range(num_clbits)))
return new_circuit
return batch_circuits
Loading