Skip to content
Closed
Show file tree
Hide file tree
Changes from 26 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
174 changes: 131 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,144 @@ def _initialize_experiment_data(

return experiment_data._copy_metadata()

def run_analysis(self, experiment_data, **options) -> ExperimentData:
def pre_transpile_action(self, backend: Backend):

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 is this method needed? I don't see why this is necessary and can't just be done as part of the circuits (or _circuits) method that experiments already have to implement.

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.

Yes, that is true. However this might be useful to write calibration. Because pulse gate pass allows us to automatically attach calibration with instruction schedule map, we can create the instmap and set this to transpiler options in the pre_transpile_action method. Then, we can avoid having two programing models in the same circuits method, i.e. creating schedule and circuit in the same place seems very complicated to novice users. What do you think @eggerdj ?

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'd like to move towards a seamless and easy schedule management in experiments. Currently, in experiments like Rabi, FineAmp, and Drag we have the schedule which is attached to a particular gate. However, the experiment circuits often feature other gates (e.g. a y-rotation to map a z-rotation error to a qubit population, etc.). This means that to get a self-consistent calibration framework we need to

  • Either add the different schedules manually through options which is what we are currently doing. This is unscalable, complicated, and messy.
  • Find an automatic way to attach the schedules to the circuits. Fortunately for us Terra 0.19.0 will allow us to do this with the instruction schedule map in an automatic way through a transpiller pass (thanks to Naoki).

This may justify having a pre_transpile_action. I have the following question: Could this transpiller pass be done in the current call to the transpiler with an option inst_map=inst_map_from_cals? If so I'm not sure we need pre_transpile_action if not then we need a pre_transpile_action.

@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.

Yes, this can be done from everywhere you can access to self, i.e. self.set_transpile_options(inst_map=inst_map_from_cals). However, this should be called after you get backend, and you run the transpile. Alternative approach would be write something like

class BaseCalibration:
    def circuits(self, backend):
        inst_map_from_cals = self.calibration_options().calibration.get_instmap(...)
        self.set_transpile_options(inst_map=inst_map_from_cals)
        ...

class MyCalExp(BaseCalibration):
    def circuits(self, backend):
        circus = super().circuits(backend)
        ....

this is equivalent to add pre_transpile_action. So this is mainly readability issue.

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.e. separation of experiment program and dynamic configuration.

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'd like to keep the circuits method clean of any calibration related variables. The following would be nice:

class BaseCalibrationExperiment(BaseExperiment, ABC):  # as per PR 251

    ...
    
    def pre_transpile_action():
        """Set the instruction schedule map for transpiling."""
        self.set_transpile_options(inst_map = self._cals.default_inst_map)

alternatively, without the pre_transpile_action hook we could probably do:

class BaseCalibrationExperiment(BaseExperiment, ABC):  # as per PR 251

    def __init__(self, ..., calibrations=cals):
        ...
        self.set_transpile_options(inst_map=cals.default_inst_map)

would this second option work? I think it should be okay as default_inst_map is a property and is not deep copied around so if I run my calibration experiment twice the update of the cals (and therefore the default_inst_map) should also happen. For a regular experiment that does not do any schedule management would the following workflow be sufficient?

exp = MyCoolExperiment(qubits, ...)
exp.set_transpile_options(inst_map=my_backend.defaults().instruction_schedule_map)
exp.run(my_backend)

"""An extra subroutine executed before transpilation.
Comment thread
nkanazawa1989 marked this conversation as resolved.

Note:
Comment thread
nkanazawa1989 marked this conversation as resolved.
This method may be implemented by a subclass that requires to update the
transpiler configuration based on the given backend instance,
otherwise the transpiler configuration should be updated with the
:py:meth:`_default_transpile_options` method.

For example, some specific transpiler options might change depending on the real
hardware execution or circuit simulator execution.
By default, this method does nothing.

Args:
backend: Target backend.
"""
pass

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

Note:
Comment thread
nkanazawa1989 marked this conversation as resolved.
This method may be implemented by a subclass that requires to update the
circuit or its metadata after transpilation.
Without this method, the transpiled circuit will be immediately executed on the backend.
This method enables the experiment to modify the circuit with pulse gates,
or some extra metadata regarding the transpiled sequence of instructions.

By default, this method just passes transpiled circuits to the execution chain.

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 return transpiled circuits.

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

Returns:
Transpiled circuit to execute.
"""
# Run pre transpile if implemented by subclasses.
self.pre_transpile_action(backend)

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.

As mentioned above this method seems unnecessary and can just be done in circuits and seems to have no major difference in this functions execution flow

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.

Right, but this is mainly for readability improvements. See my comments above :)


# 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_action(circuits, backend)
Comment thread
nkanazawa1989 marked this conversation as resolved.
Outdated

return circuits

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

Note:
This method may be implemented by a subclass that requires to perform
extra data processing based on the analyzed experimental result.

Note that the analysis routine will not complete until the backend job
is executed, and this method will be called after the analysis routine
is completed though a handler of the experiment result will be immediately
returned to users (a future object). This method is automatically triggered
when the analysis is finished, and will be processed in background.

If this method updates some other (mutable) objects, you may need manage
synchronization of update of the object data. Otherwise you may want to
call :meth:`block_for_results` method of the ``experiment_data`` here
to freeze processing chain until the job result is returned.

By default, this method does nothing.

Args:
experiment_data: A future object of the experimental result.
"""
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_action(experiment_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.

Suggested change
self.post_analysis_action(experiment_data)
self._post_analysis_action(experiment_data)

If the callback is handled in the run method as was originally done instead of being moved to this method, then calling post_analysis_action will automatically be run as part of the run analysis callback when doing experiment.run.

Maybe we could rework how the experiment data callback works so that run_analysis could be added as a callback to an existing container if its still running the experiment.

@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.

The motivation of including hooks in this method is to reduce maintenance overhead in the composite experiment. If we call hooks directly in the run method, we need to override entire run method in the composite experiment. In this implementation, composite just need to update run_transpile and (optionally) run_analysis method.

@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.

Done in bf34b77 (personally I don't prefer this cyclic reference, i.e. experiment.analysis.run -> experiment_data -> experiment_data.experiment)


return experiment_data

@property
Expand Down Expand Up @@ -335,10 +427,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
47 changes: 47 additions & 0 deletions qiskit_experiments/framework/common_operations.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# This code is part of Qiskit.
#
# (C) Copyright IBM 2021.
#
# This code is licensed under the Apache License, Version 2.0. You may
# obtain a copy of this license in the LICENSE.txt file in the root directory
# of this source tree or at http://www.apache.org/licenses/LICENSE-2.0.
#
# Any modifications or derivative works of this code must retain this
# copyright notice, and modified files need to carry a notice indicating
# that they have been altered from the originals.

"""A collection of common operation callback in execution chain."""


from qiskit.providers import Backend
from qiskit.test.mock import FakeBackend

from .base_experiment import BaseExperiment


def apply_delay_validation(experiment: BaseExperiment, backend: Backend):

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.

Does this need to be in framework? It really feels like all the content of this function is something that should be handled correctly by terra transpiler/scheduler.

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.

Half of this code is now automatically handled by transpiler since backends recently started to report the timing constraints information. However, we still need to set scheduling options, i.e. transpiler options of scheduling_method. This can be a default transpiler options of corresponding experiments, however, a fake backend doesn't require scheduling since it implicitly calls simulator and no timing constraints there. But we can still set scheduling option for simulator backend at the expense of transpiler overhead (this is heavy compute pass since it recreates DAG circuits).

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 tried not to change currently implemented code, but I can also update that code in this PR. There are two options

  • Keep this pre-transpile. Remove the code to set acquire_alignment.
  • Entirely remove this code and set "scheduling_method": "asap" to default transpiler options. This will induce non-necessary overhead in the simulator execution.

"""Enable delay duration validation to conform to backend alignment constraints.

Args:
experiment: Experiment instance to run.
backend: Target backend.
"""
is_simulator = getattr(backend.configuration(), "simulator", False)

if not is_simulator and not isinstance(backend, FakeBackend):
timing_constraints = getattr(
experiment.transpile_options.__dict__, "timing_constraints", {}
)

# alignment=16 is IBM standard. Will be soon provided by IBM providers.
# Then, this configuration can be removed.
timing_constraints["acquire_alignment"] = getattr(
timing_constraints, "acquire_alignment", 16
)

scheduling_method = getattr(
experiment.transpile_options.__dict__, "scheduling_method", "alap"
)
experiment.set_transpile_options(
timing_constraints=timing_constraints, scheduling_method=scheduling_method
)
Loading