Skip to content
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

Open
wants to merge 111 commits into
base: main
Choose a base branch
from

Conversation

Altanali
Copy link
Contributor

@Altanali Altanali commented Jul 10, 2024

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

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ltnln added 30 commits June 2, 2024 23:35
…f attribute strings; introduce PyTket-to-QASM translations
…nstead of gate name strings in GateFidelity data class.
@speller26 speller26 requested a review from maolinml July 15, 2024 21:06
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ee63777) to head (baccbb1).

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.
📢 Have feedback on the report? Share it here.

@Altanali Altanali changed the title feat: Introduce Validating Emulators with Noise Models to the SDK feature: Introduce Validating Emulators with Noise Models to the SDK Jul 15, 2024
Copy link
Contributor

@maolinml maolinml left a 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

  1. For connectivity graph, can we use indirected graph instead of directed graph?
  2. 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.

>>> print(result.result().measurement_counts)

Returns:
Emulator: An emulator for this device, if this is not a simulator device.
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed - added!

"""
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.
Copy link
Contributor

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.

Copy link
Contributor Author

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 Show resolved Hide resolved
) -> 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
Copy link
Contributor

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

Copy link
Contributor Author

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.

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
Copy link
Contributor

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.

Copy link
Contributor Author

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:
Copy link
Contributor

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.

Copy link
Contributor

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.

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`.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

"""
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

"retruning", typo?

Copy link
Contributor Author

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):
Copy link
Contributor

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?

Copy link
Contributor Author

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):
Copy link
Contributor

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.

  1. Does the following validation satisfies this requirement? I wasn't able to tell yet.
  2. 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. 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).

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

Copy link
Member

@speller26 speller26 left a 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:
Copy link
Member

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:
Copy link
Member

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:
Copy link
Member

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

Copy link
Contributor Author

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
Copy link
Member

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(...):
    ...

Copy link
Contributor Author

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(
Copy link
Member

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.

Copy link
Contributor Author

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.

The following gate duration values are not available through Braket device
calibration data and must be hardcoded.
"""
QPU_GATE_DURATIONS = {
Copy link
Member

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.

self._validate_two_qubit_specs()


def create_device_noise_model(properties: DeviceCapabilities, arn: str) -> NoiseModel:
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

Comment on lines 10 to 14
"""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.
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
"""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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

Args:
circuit (Circuit): The Braket circuit whose gate operations to
validate.
"""
Copy link
Member

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

Copy link
Contributor Author

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.
or a valid DiGraph or dict representation of a connectivity graph is not provided.
"""

if not (connectivity_graph or fully_connected):
Copy link
Member

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):
Copy link
Member

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?

Copy link
Contributor Author

@Altanali Altanali Jul 30, 2024

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")
Copy link
Member

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")

Copy link
Contributor Author

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!

from braket.registers.qubit_set import QubitSet


class ConnectivityCriterion(EmulatorCriterion):
Copy link
Member

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/__init__.py Outdated Show resolved Hide resolved
src/braket/emulators/emulator.py Outdated Show resolved Hide resolved
src/braket/emulators/emulator_interface.py Outdated Show resolved Hide resolved
src/braket/aws/aws_emulator_helpers.py Outdated Show resolved Hide resolved
…Emulator._get_local_simulator_backend overwriting user provided backend name.
…exception raised by an EmulatorPass in order to modify the message with the Emulator name.
Comment on lines 14 to 15
supported_gates: Optional[Iterator[str]] = None,
native_gates: Optional[Iterator[str]] = None,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
supported_gates: Optional[Iterator[str]] = None,
native_gates: Optional[Iterator[str]] = None,
supported_gates: Optional[Iterable[str]] = None,
native_gates: Optional[Iterable[str]] = None,

Copy link
Contributor Author

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!

src/braket/aws/aws_device.py Outdated Show resolved Hide resolved
return GateConnectivityValidator(gate_connectivity_graph)


def _get_qpu_gate_translations(
Copy link
Member

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

Comment on lines 33 to 38
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)
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to frozenset!

Copy link
Member

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.

Copy link
Contributor Author

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!

@speller26
Copy link
Member

Looking good to me, let's see if @krneta has any feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants