-
Notifications
You must be signed in to change notification settings - Fork 118
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
feature: Introduce Validating Emulators with Noise Models to the SDK #1017
base: main
Are you sure you want to change the base?
Conversation
…OpenQASM3.0 sources
…f attribute strings; introduce PyTket-to-QASM translations
…nherit from EmulatorPass
…nstead of gate name strings in GateFidelity data class.
…s using QPU device properties
…l inside AwsDevice
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1017 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 135 149 +14
Lines 9004 9495 +491
Branches 2019 2127 +108
==========================================
+ Hits 9004 9495 +491 ☔ View full report in Codecov by Sentry. |
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.
Very good work! High level comment
- For connectivity graph, can we use indirected graph instead of directed graph?
- You have spent lots of effort in handling the differences across different devices. Let us add some comments in the code so that others can understand the logic better, which will also help future maintenance and developments.
src/braket/aws/aws_device.py
Outdated
>>> print(result.result().measurement_counts) | ||
|
||
Returns: | ||
Emulator: An emulator for this device, if this is not a simulator device. |
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.
Should we add in "Returns" that an error will be thrown if the device is not a QPU?
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.
Agreed - added!
src/braket/aws/aws_device.py
Outdated
""" | ||
A device emulator mimics the restrictions and noise of an AWS QPU by validating and | ||
compiling programs before running them on a simulated backend. An emulator can be used | ||
as a soft check that a program can run on an AwsDevice. |
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.
Let's emphasize that the emulator is tied to the QPU, or 1-1 mapped to the QPU. So maybe let's say "A device emulator mimics the restrictions and noise of the AWS QPU by validating and compiling programs before running them on a simulated backend. An emulator can be used as a soft check that a program can run on the target AwsDevice.
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 a good point, I've made the proposed changes to clarify that the emulator's are specific to the exact AwsDevice being used.
src/braket/aws/aws_device.py
Outdated
) -> None: | ||
""" | ||
Runs all non-modifying emulator passes on the input program and raises an | ||
error if any device-specific criterion are not met by the program. If the |
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 super important: can we check the usage of "criterion" vs "criteria" in doc-string and function names? I thought the former is singular and the latter is plural, but I am not the best person to check the grammar..
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.
Your call out is correct, this was a mistake on my part! "Criteria" should have been used here (and in line 926) where we are referencing multiple device constraints/criteria.
src/braket/aws/aws_device.py
Outdated
inputs: Optional[dict[str, float]] = None, | ||
) -> QuantumTask: | ||
"""Emulate a quantum task specification on this quantum device emulator. | ||
A quantum task can be a circuit or an annealing problem. Emulation |
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 the task be annealing problem? If yes, why do we want to do annealing, given that Dwave is long gone.
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 task cannot be an annealing problem, this was a typo, thank you!
|
||
class ConnectivityCriterion(EmulatorCriterion): | ||
""" | ||
args: |
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 argument directed
is missing in the docstring.
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.
Also I am a bit lost by the different combination of the arguments. It would be good to summarize the valid argument combinations, or try to somehow simplify this class.
src/braket/emulators/emulator.py
Outdated
task_specification (Union[Circuit, OpenQasmProgram]): Specification of a quantum task | ||
to run on device. | ||
shots (Optional[int]): The number of times to run the quantum task on the device. | ||
Default is `None`. |
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.
Shouldn't it be "Default is 0" for shots?
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.
Shots default=None follows the default value for the shots argument in the run methods for aws_device
and local_simulator
.
src/braket/emulators/emulator.py
Outdated
""" | ||
Passes the input program through all EmulatorPass objects contained in this | ||
emulator and applies the emulator's noise model, if it exists, before | ||
retruning the compiled program. |
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.
"retruning", typo?
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.
Good catch! Fixed!
emulator's validation passes. | ||
""" | ||
for emulator_pass in self._emulator_passes: | ||
if isinstance(emulator_pass, EmulatorCriterion): |
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.
Why do we need this if-statement?
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.
In validate()
, we only want to run the EmulatorCriterion
emulator passes that are only used for validation, and are non-modifying; this check is used to avoid modifying EmulatorPass
instances (currently, there are none, but if we have mapping/routing or nativization passes, we will want to avoid applying those to the Program when calling validate()
).
circuit (Circuit): The Braket circuit whose gates to validate. | ||
""" | ||
idx = 0 | ||
while idx < len(circuit.instructions): |
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.
High level questions. I thought if there is a verbatim box in the circuit, then all the gates have to be native gates.
- Does the following validation satisfies this requirement? I wasn't able to tell yet.
- Can we simply check if there is a verbatim box in the circuit, and check against either the supported gate set and native gate set accordingly for all the gates?
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.
- If there is a verbatim box in the circuit, all the gates don't have to be native gates - just the ones in the verbatim box. Supported gates outside the verbatim box can still be nativized without any issue!
However, if there is a verbatim box, then all qubit references must be to hardware qubits (this is checked in ConnectivityCriterion
).
- Because of the comment in response to (1), we need to use both the supported gate set and the native gate set when validating a circuit.
…to ensure user-provided undirected graphs have symmetric forwards/backwards edges
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.
Still reviewing; great work though!
task_specification = emulator_pass(task_specification) | ||
return task_specification | ||
|
||
def run_validation_passes(self, task_specification: ProgramType) -> None: |
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.
Might be better to call this validate
def __init__(self, emulator_passes: Iterable[EmulatorPass] = None): | ||
self._emulator_passes = emulator_passes if emulator_passes is not None else [] | ||
|
||
def run_program_passes(self, task_specification: ProgramType) -> ProgramType: |
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.
May just run_passes
?
) | ||
|
||
|
||
def create_qubit_count_criterion(properties: DeviceCapabilities) -> QubitCountCriterion: |
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.
Just qubit_count_criterion
is fine; same goes for other create methods
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.
Updated!
return GateCriterion(supported_gates=supported_gates, native_gates=native_gates) | ||
|
||
|
||
@singledispatch |
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.
singledispatch
produces really ugly documentation when used on public functions; only use it on private methods, like so:
def connectivity_criterion(...):
return _connectivity_criterion(...)
@singledispatch
def _connectivity_criterion(...):
...
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.
Updated! Thank you for this callout, I wasn't aware of that interaction!
return GateConnectivityCriterion(gate_connectivity_graph) | ||
|
||
|
||
def get_qpu_gate_translation( |
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.
Are the names not already standardized across QPUs? This shouldn't be necessary.
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 are a few places where translation to a Braket gate name was necessary, i.e. gate names in Rigetti calibration data ("CPHASE" is used instead of the standard "cphaseshift") or in IonQs native gate set (translating "GPI"/"GPI2" to "GPi"/"GPi2"). I felt this was a little cleaner than doing translations in each place required.
src/braket/aws/aws_noise_models.py
Outdated
The following gate duration values are not available through Braket device | ||
calibration data and must be hardcoded. | ||
""" | ||
QPU_GATE_DURATIONS = { |
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.
Make this private; in fact, make things private whenever possible.
src/braket/aws/aws_noise_models.py
Outdated
self._validate_two_qubit_specs() | ||
|
||
|
||
def create_device_noise_model(properties: DeviceCapabilities, arn: str) -> NoiseModel: |
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.
device_noise_model
is sufficient
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.
Updated!
"""Runs the emulator pass on the provided program. | ||
Args: | ||
program (ProgramType): The program to run the emulator pass on. | ||
Returns: | ||
ProgramType: The program after the emulator pass has been applied. |
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.
nit
"""Runs the emulator pass on the provided program. | |
Args: | |
program (ProgramType): The program to run the emulator pass on. | |
Returns: | |
ProgramType: The program after the emulator pass has been applied. | |
"""Runs the emulator pass on the provided program. | |
Args: | |
program (ProgramType): The program to run the emulator pass on. | |
Returns: | |
ProgramType: The program after the emulator pass has been applied. |
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.
Updated!
src/braket/emulators/emulator_passes/criteria/connectivity_criterion.py
Outdated
Show resolved
Hide resolved
Args: | ||
circuit (Circuit): The Braket circuit whose gate operations to | ||
validate. | ||
""" |
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.
Should also add a Raises
section
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.
Added a Raises section to all methods where an exception is raised!
…e in aws_emulator_helpers; general documentation cleanup.
src/braket/emulators/emulator_passes/criteria/connectivity_criterion.py
Outdated
Show resolved
Hide resolved
or a valid DiGraph or dict representation of a connectivity graph is not provided. | ||
""" | ||
|
||
if not (connectivity_graph or fully_connected): |
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.
Maybe also raise an error if both are provided?
ProgramType = TypeVar("ProgramType") | ||
|
||
|
||
class EmulatorPass(ABC): |
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.
Should be EmulatorPass(ABC, Generic[ProgramType])
; also, how about EmulationPass
?
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.
Updated in b698648!
from abc import ABC, abstractmethod | ||
from typing import TypeVar | ||
|
||
ProgramType = TypeVar("ProgramType") |
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.
For generics, it's fine to use just T = TypeVar("T")
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've opted to use "ProgramType
" just as it makes it clearer throughout code/documentation that an EmulatorPass expects some generic program as input - let me know if this approach makes sense!
src/braket/emulators/emulator_passes/criteria/emulator_criterion.py
Outdated
Show resolved
Hide resolved
src/braket/emulators/emulator_passes/criteria/gate_connectivity_criterion.py
Outdated
Show resolved
Hide resolved
src/braket/emulators/emulator_passes/criteria/connectivity_criterion.py
Outdated
Show resolved
Hide resolved
from braket.registers.qubit_set import QubitSet | ||
|
||
|
||
class ConnectivityCriterion(EmulatorCriterion): |
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.
ConnectivityCriterion(EmulatorCriterion[Circuit])
; also, how about naming all the Criterion
subclasses __Validators
?
src/braket/emulators/emulator_passes/criteria/gate_criterion.py
Outdated
Show resolved
Hide resolved
… program type in EmulatorPasses
…n a designated directory.
…Emulator._get_local_simulator_backend overwriting user provided backend name.
…dk-python into only_validation
…exception raised by an EmulatorPass in order to modify the message with the Emulator name.
…unsupported device capabilities
supported_gates: Optional[Iterator[str]] = None, | ||
native_gates: Optional[Iterator[str]] = None, |
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.
supported_gates: Optional[Iterator[str]] = None, | |
native_gates: Optional[Iterator[str]] = None, | |
supported_gates: Optional[Iterable[str]] = None, | |
native_gates: Optional[Iterable[str]] = None, |
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.
🤦♂️ Thanks for catching this, fixed!
return GateConnectivityValidator(gate_connectivity_graph) | ||
|
||
|
||
def _get_qpu_gate_translations( |
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'll be very happy when this is no longer needed
self._supported_gates = set(BRAKET_GATES[gate.lower()] for gate in supported_gates) | ||
except KeyError as e: | ||
raise ValueError(f"Input {str(e)} in supported_gates is not a valid Braket gate name.") | ||
|
||
try: | ||
self._native_gates = set(BRAKET_GATES[gate.lower()] for gate in native_gates) |
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.
nit: frozenset
instead of set
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.
Changed to frozenset
!
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 general enough that it perhaps belongs in its own module (maybe passes
or transpilation
)? The class itself might even be named just Pass
; at the very least, EmulationPass
is to specific for what it's actually capable of.
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've moved it to a module called "passes" under the name "BasePass" (to try and avoid collisions with the pass
keyword even if the casing is different). Now, ValidationPass inherits directly from BasePass!
Looking good to me, let's see if @krneta has any feedback! |
Issue #, if available:
Description of changes:
This PR introduces the notion of device emulators and emulation passes to the BDK; device emulators provide some form of program compilation and validation and may apply a noise model to the program before simulating the program on the BDK LocalSimulator.
AwsDevice specific emulators are created based on the device properties and capabilities; noise models for gate-based QPUs are created based on calibration data retrieved from Braket service when creating an AwsDevice.
Testing done:
Unit testing covers 99% of emulator passes and noise model generation based on AwsDevice calibration data. (TODO: Complete unit tests for 100% coverage).
Merge Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.