feature: Gate and Circuit inversion #311

speller26 · 2022-03-10T00:46:10Z

Introduces the ability to take adjoints of Gates, Instructions and Circuits.

Issue #, if available:

Description of changes:

Testing done:

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

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

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.

Introduces the ability to take adjoints of Gates, Instructions and Circuits. Taking the adjoint is a lazy operation; the actual gates that invert the original gate do not appear until `to_ir` is called.

codecov · 2022-03-10T00:51:15Z

Codecov Report

Merging #311 (8fbc65d) into main (614029a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #311   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           66        66           
  Lines         4579      4653   +74     
  Branches       640       645    +5     
=========================================
+ Hits          4579      4653   +74

Impacted Files	Coverage Δ
src/braket/circuits/angled_gate.py	`100.00% <100.00%> (ø)`
src/braket/circuits/circuit.py	`100.00% <100.00%> (ø)`
src/braket/circuits/compiler_directive.py	`100.00% <100.00%> (ø)`
src/braket/circuits/compiler_directives.py	`100.00% <100.00%> (ø)`
src/braket/circuits/gate.py	`100.00% <100.00%> (ø)`
src/braket/circuits/gates.py	`100.00% <100.00%> (ø)`
src/braket/circuits/instruction.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 614029a...8fbc65d. Read the comment docs.

src/braket/circuits/compiler_directive.py

src/braket/circuits/instruction.py

Co-authored-by: Abe Coull <[email protected]>

test/unit_tests/braket/circuits/test_angled_gate.py

krneta · 2022-03-11T21:50:04Z

src/braket/circuits/circuit.py

+    def adjoint(self) -> Circuit:
+        circ = Circuit([instr.adjoint() for instr in reversed(self.instructions)])
+        for result_type in self._result_types:
+            circ.add_result_type(result_type)


I always worry that there's data we're forgetting to copy over. For example, do we need to copy over the parameters?

Nope, that's included in the gates. Right now, there are only two things you can add to a circuit: instructions and result types (and other circuits, which are made up of these)

src/braket/circuits/compiler_directive.py

krneta · 2022-03-11T22:01:20Z

src/braket/circuits/gate.py

+        new._adjoint = not self._adjoint
+        return new
+
+    def adjoint_list(self) -> List[Gate]:


Isn't it weird to have both adjoint and adjoint_list? When would we know which one to use? Why not just always use adjoint_list?

All adjoint does is mark a gate as an adjoint; it doesn't have anything to do with the actual implementation of adjoints. adjoint_list, on the other had, is a definition of the adjoint. In principle, adjoint_list isn't necessary at all; the adjoint implementation could be handled by the device, but we currently have no way of communicating that.

Could we differentiate the names more to make this difference more obvious?

adjoint_expansion?

src/braket/circuits/gate.py

krneta · 2022-03-11T22:02:51Z

src/braket/circuits/gate.py

+        Returns:
+            List[Gate]: The gates comprising the adjoint of this gate.
+        """
+        raise NotImplementedError(f"Gate {self.name} does not have adjoint implemented")


I'm confused why this isn't:
return [self.adjoint()]

See comment about difference between adjoint and adjoint_list

krneta · 2022-03-11T22:04:26Z

src/braket/circuits/gate.py

@@ -49,18 +73,35 @@ def to_ir(self, target: QubitSet) -> Any:
        Returns:
            IR object of the quantum operator and target
        """
+        if self._adjoint:
+            # Use adjoint list of original gate
+            return [elem.to_ir(target) for elem in self.adjoint().adjoint_list()]


.adjoin().adjoint_list() doesn't make sense to me from the description of these functions.

adjoint_list will often contain copies of self, and we don't want to recursively call to_ir on self because it'll never end. As well, semantically speaking, we want the list of gates that form the adjoint of the original gate, not the adjoint of the adjoint.

Isn't there a possibility of an infinite loop here regardless? What if this is class A, and adjoint_list returns B.adjoint(), and adjoint_list of that returns A.adjoint()? Can we do some validation in the loop to make sure the returned gates are not self and not adjoint themselves?

I think the best way to go about this is to throw an exception if _adjoint is True for any gate in adjoint_list, and include in the adjoint_list doc that the adjoint list must consist only of non-adjoint gates.

src/braket/circuits/gate.py

krneta · 2022-03-11T22:06:08Z

src/braket/circuits/gates.py

    """Hadamard gate."""

    def __init__(self):
        super().__init__(qubit_count=None, ascii_symbols=["H"])

-    def to_ir(self, target: QubitSet):
+    def _to_ir(self, target: QubitSet):


Can we just change all these to private? Is that a breaking change?

Won't be breaking because they all inherit from Gate, where the public to_ir is defined.

krneta · 2022-03-11T22:07:01Z

src/braket/circuits/gates.py

@@ -39,16 +40,31 @@
    3. Register the class with the `Gate` class via `Gate.register_gate()`.
 """

+
+class _FiniteOrderGate(Gate, ABC):


Missing docs.

Why are these new superclasses needed?

I like having these because many gates fall into this category. I think it's much better than pasting

def adjoint_list(self): return [self]

for all involutory gates, and

def adjoint_list(self): return [self, self, self] # or more, etc

for other orders. As well, it helps prevent stop a developer from missing or incorrectly implementing these methods for new gates in the same category.

I suggest we remove both FiniteOrderGate and HermitianGate. It's probably more confusing for developers to use instead of just defining the adjoint operator themselves.

I'm fine with removing _finiteOrderGate, as there aren't too many instances of these yet, and it's less obvious than _HermitianGate. However, _HermitianGates are pretty ubiquitous, and the meaning of the class is pretty clear from its name.

src/braket/circuits/instruction.py

krneta · 2022-03-11T22:20:35Z

test/unit_tests/braket/circuits/test_ascii_circuit_diagram.py

+    expected = (
+        "T  : |     0      |  1  |",
+        "                         ",
+        "q0 : -(Rz(3.14))^†-(S)^†-",


Rather than two characters it might look nicer if we just use an asterisk instead?

While an asterisk is standard for adjoints in math, in physics it just means the complex conjugate, which is not what we want. I'm fine with just having the dagger without the ^ though

src/braket/circuits/angled_gate.py

krneta · 2022-03-14T17:19:39Z

src/braket/circuits/gate.py

+        """Tuple[str, ...]: Returns the ascii symbols for the quantum operator."""
+        if self._adjoint:
+            return tuple(
+                # "C" stands for control, and doesn't need a dagger symbol


But the ascii symbols are determined by subclasses, it seems like this is information leaking to know what one of the subclass ascii symbols would be.

Why can't the subclass just override this method?

I want to avoid having to copy the if statement for every subclass, as it's easy for a developer to forget to add the branch.

kshitijc · 2022-03-24T19:29:49Z

src/braket/circuits/gates.py

@@ -79,11 +107,11 @@ def h(target: QubitSetInput) -> Iterable[Instruction]:
 Gate.register_gate(H)


-class I(Gate):  # noqa: E742, E261
+class I(_FiniteOrderGate):  # noqa: E742, E261


Isn't I hermitian ?

I has order 1, which implies Hermiticity, but here we do one better by simply not adding a gate at all, as opposed to a single gate. I don't have a problem with making it explicitly Hermitian.

It's this kind of discussion that makes me not want to have _HermitianGate in the first place; I don't think it adds too much value and just makes development slower.

licedric · 2022-04-28T15:44:12Z

src/braket/circuits/angled_gate.py

@@ -87,6 +88,16 @@ def bind_values(self, **kwargs):
        """
        raise NotImplementedError

+    def adjoint(self) -> List[Gate]:


This is true for all of our current gates but might not be in the future. Is there a way to catch it?

This is the default behavior, and future gates could just override it.

* feat: add queue position for tasks (#299) * feat: add queue position for tasks * update docstring for queue_position * update docstrings * update package_name * update to enums and dataclass * test: add integ test for task queue_position (#300) * refactor: dataclass and file naming for queue info (#301) * refactor: dataclass and file naming for queue info * apply suggestions * update: task queue position after refactor (#309) * update: task queue position after refactor * add queue_position type hint details, change order of info * feat: queue position for hybrid jobs (#302) * feat: queue position for hybrid jobs * handle message return * add docstring changes * update docstring, and return None * add context in dataclass * update docstrings * indent fix * test: add integ test for jobs queue position (#310) * feat: queue position for hybrid jobs * handle message return * add docstring changes * update docstring, and return None * add context in dataclass * test: add integ test for jobs queue position * remove comment * minor fix * remove unnecessary branching * fix docstring merge * remove dataclass redefinition after merge * feat: queue depth for devices (#306) * feat: queue depth for devices (dataclass version) * add unit-test for queue depth * modify docstrings, remove helper funcs * add more info to docstrings * minor test edit * docstrings * indent * update QueuePriority to QueueType * test: add integ test for queue_depth (#311) * refactor: job and quantum_task keywords for queue_depth (#312) * refactor: job and quantum_task keywords for queue_depth * Update src/braket/aws/queue_information.py Co-authored-by: Kshitij Chhabra <[email protected]> --------- Co-authored-by: Kshitij Chhabra <[email protected]> * deps: update boto3 version for queue visibility (#319) * sync: public-main changes into feature/queue_visibility (#320) * feat: add Aria2 enum (#653) Co-authored-by: Viraj Chaudhari <[email protected]> Co-authored-by: Cody Wang <[email protected]> * infra: bump actions/checkout from 3.6.0 to 4.0.0 (#696) Bumps [actions/checkout](https://github.com/actions/checkout) from 3.6.0 to 4.0.0. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@f43a0e5...3df4ab1) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * prepare release v1.55.0 * update development version to v1.55.1.dev0 * Revert "update: restricting parameter names to not collide with ones we use for OpenQASM generation. (#675)" (#701) This reverts commit b158736. * infra: update codeowner file to amazon-braket/braket-dev (#699) Co-authored-by: Abe Coull <[email protected]> * doc: Replace aws org with amazon-braket (#705) * prepare release v1.55.1 * update development version to v1.55.2.dev0 * doc: change the sphinx requirement to be greater than 7.0.0 (#704) Co-authored-by: Cody Wang <[email protected]> * doc: add code contributors to the readme (#703) Co-authored-by: Cody Wang <[email protected]> * doc: Remove trailing backquotes (#706) * infra: update the pre-commit hook with linters (#678) * infra: update the pre-commit hook with linters and secrets check Co-authored-by: Abe Coull <[email protected]> Co-authored-by: Cody Wang <[email protected]> * prepare release v1.55.1.post0 * update development version to v1.55.2.dev0 --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: ashlhans <[email protected]> Co-authored-by: Cody Wang <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: ci <ci> Co-authored-by: Milan <[email protected]> Co-authored-by: Angela Guo <[email protected]> Co-authored-by: Abe Coull <[email protected]> Co-authored-by: Abe Coull <[email protected]> * Revert "sync: public-main changes into feature/queue_visibility (#320)" This reverts commit be6460c. * update github script * minor fix --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Kshitij Chhabra <[email protected]> Co-authored-by: ashlhans <[email protected]> Co-authored-by: Cody Wang <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Milan <[email protected]> Co-authored-by: Angela Guo <[email protected]> Co-authored-by: Abe Coull <[email protected]> Co-authored-by: Abe Coull <[email protected]>

feature: Gate and Circuit inversion

382ec52

Introduces the ability to take adjoints of Gates, Instructions and Circuits. Taking the adjoint is a lazy operation; the actual gates that invert the original gate do not appear until `to_ir` is called.

speller26 requested review from kshitijc, krneta, licedric and mbeach-aws March 10, 2022 00:46

speller26 requested a review from a team as a code owner March 10, 2022 00:46

AbeCoull reviewed Mar 10, 2022

View reviewed changes

src/braket/circuits/compiler_directive.py Outdated Show resolved Hide resolved

AbeCoull reviewed Mar 10, 2022

View reviewed changes

src/braket/circuits/instruction.py Outdated Show resolved Hide resolved

Update src/braket/circuits/instruction.py

ff2cc5c

Co-authored-by: Abe Coull <[email protected]>

AbeCoull reviewed Mar 10, 2022

View reviewed changes

test/unit_tests/braket/circuits/test_angled_gate.py Outdated Show resolved Hide resolved