[UnitaryHack] Gate Decomposition(#90) added and tested #111

Gmontes01 · 2023-06-14T08:51:13Z

Summary

Expands on gate decomposition as described in the thread (#90) and fixes the critiques posed in prior PR (#109) including

translation of unitary gates now do not create a phase shift while implementing the operation efficiently(<=3 gates per unitary op)
poorly named "list_list" was removed
decompose_fully() function was replaced with a call to the qiskit transpiler instead with basis gates set as the set of gates/operations which can be directly translated using the dictionary.

    quantum_circuit = Circuit()
    if not (
        {gate.name for gate, _, _ in circuit.data}.issubset(translatable_qiskit_gates)
    ):
        circuit = transpile(circuit, basis_gates=translatable_qiskit_gates)

transpilation returns circuits which may have a shift to its global_phase so if there is a shift in global phase of the circuit a warning is printed letting the user know that the global_phase in the translation is not consistent with that of qiskit. Transpilation prevents any given gate from being decomposed too much since the tranpiler would simply stop decomposing any gate when it can be expressed as the transpilers basis gates.

    if circuit.global_phase > _EPS:
        warnings.warn("Circuit transpilation resulted in global phase shift")

elif branch to handle 'reset' operation was created in convert_qiskit_to_braket_circuit which returns an error since the provider is not currently capable of converting a 'reset' operation into braket

        elif name == "reset":
            raise NotImplementedError(
                "reset operation not supported by qiskit to braket adapter"
            )

Details and comments

Use of transpilation made the old tests for successful conversion of standard and exponential gates trivial so those tests were updated. Now the tests work by confirming a similar behavior for a qiskit gate and its braket translation by measuring qubit states. This revealed the bug where specific gates(rxz,ecr,exponentials) when transpiled and converted to braket end up swapping the first and second qubit of the operation. For example giving the probability of '01' in the qiskit circuit to '10' in that circuits braket translation. I ended up separating and testing these types of gates separately with well behaved standard gates placed in the standard_gates list and the gates which flip qubits around in flipped_gates with both having their own unittest to make sure the flipped gates do in fact have the behavior I described.

Also note the CUGate has been commented out of the standard_gates list because its parameters cannot be assigned due to an unresolved issue with qiskit's assign_parameter method. It can be added back into the test quickly by uncommenting it and testing if an updated version of qiskit has resolved this issue.

jcjaskula-aws · 2023-06-15T15:17:08Z

tests/providers/test_adapter.py

+    CRZGate(Parameter("ϴ")),
+    CSwapGate(),
+    CSXGate(),
+    # CUGate(Parameter("ϴ"), Parameter("φ"), Parameter("λ"), Parameter("γ")), qiskit's


If CUGate is not translatable, it should raise an error in the code and we should have a test that monitor that the exception has been thrown (pytest.raises).
We should not have commented code.

commented code removed and pytest was added to standard gate test for when CUGate is translated

jcjaskula-aws · 2023-06-15T15:17:58Z

tests/providers/test_adapter.py

+    RYGate(Parameter("ϴ")),
+    RYYGate(Parameter("ϴ")),
+    RZZGate(Parameter("ϴ")),
+    # RZXGate(Parameter("ϴ")), braket treats q0 as q1 and q0 as q1 after translating with this gate


Avoid commented code. Please see my comments concerning flipped_gate too.

commented code was removed and handled in the update

jcjaskula-aws · 2023-06-15T15:26:25Z

tests/providers/test_adapter.py

+    ZGate(),
+]
+
+flipped_gates = [RZXGate(Parameter("ϴ")), ECRGate()]


I'm concerned that we have a separate case for these gates. I get the issue here (note of https://qiskit.org/documentation/stubs/qiskit.circuit.library.RZXGate.html#qiskit.circuit.library.RZXGate). I think the code should make sure that the right braket gate is applied corresponding to what the qiskit gate is expect to do, i.e qiskit_rxz(0,1) -> braket_rxz(1,0).
It might require that the transpiler ignores the RZX gate and we do the decomposition manually with ECRs.

Wouldn't we need to decompose manually the other way around with us decomposing the ECR gate into the RZX gates?

The remedy you suggested was exactly what was needed to have translated circuits produce correct state vectors. Sadly however the transformation which fixes the resultant state vector of a given translated circuit messed up the actual result that is yielded by simulating the circuit. I believe there is an inconsistency with how the braket versions of the ecr, rzx, and paulievolution gates handle state vectors that should investigated as issues in the braket sdk since similar state vectors would yield differing simulated results and similar simulated results required different state vectors.

@jcjaskula-aws, @an investigation into this issue may involve making a contribution to the braket sdk so maybe this should be raised as a seperate issue

Got it. Feel free to create a minimum viable example that involves only the Braket SDK and raise an issue at https://github.com/aws/amazon-braket-sdk-python.
On the other hand, I'm not sure I'm getting why the resultant state vector is ok but no the simulation results. Between what would be the inconsistency?

My current suspicion is that the braket handling of the tranformations of these gates may have two different transformations applying to two different simulations models of the quantum system. I imagine the model used to compute probabilities for shots(DAG and/or density matrix) applies a transformation which is consistent with qiskit's computations of the shots but the matrix used to compute the transformation of the state vector function like you described

qiskit_rxz(0,1) -> braket_rxz(1,0)

which would be evidenced by my testing in notebooks. When I apply the transformation qiskit_rxz(0,1) -> braket_rxz(1,0) I get the following results.

and when I apply the direct translation qiskit_rxz(0,1) -> braket_rxz(0,1) I get the following result

The analogous is true for ecr and paulievolution gates which I saw when trying to implement a statevector based test for standard and exponential gates. However, this problem does not exist for any other gates which excludes problems like braket generally inverting qubits in gates or the same name being given to different gates between qiskit and braket.

The two screenshots are the same, can you share what you were expected in the second one?

Bumping this discussion up. Could you share the MVE that gets you this and/or update the second screenshot.

If you look closely you will see that in the first screen shot the outcomes are shown to be different when you look at the outcomes and their printed results and that the state vector equivalence evaluates to True which should not be the case since two equivalent state vectors should give the same output when measured.

In the second screenshot the opposite is true. The results from measuring 1000 shots are roughly the same but the vectors are evaluated as being different. This is possible given a relative phase shift which would change the state vector of quantum state but not the magnitudes. A relative phase shift does not however explain the first screen shot thus my suspicion.

MVE incoming

My bad, I indeed missed the difference. Thanks for pointing it out.

jcjaskula-aws · 2023-06-15T15:30:11Z

tests/providers/test_adapter.py

+        qiskit_circuit = QuantumCircuit(1)
+        backend = BasicAer.get_backend("statevector_simulator")
+        device = LocalSimulator()
+        for _ in range(8):


Tests should not be probabilistic, there should not need to test the same circuit 8 times.

This test does not test the same circuit 8 times but rather adds an additional u gate on top of the already existing circuit 8 times. Thus checking the final state of the circuits ...
-u-
-u-u-
-u-u-u-
...
-u-u-u-u-u-u-u-u-
always match

The updated test_adapter file should only test applying the u gate once

Thanks for the explanation, I indeed miss the fact that the circuit was not reinitialized. The test looks better in its simpler form.

jcjaskula-aws · 2023-06-15T15:31:36Z

tests/providers/test_adapter.py

+                            f"and absolute difference {abs_diff}. Original values {values}",
+                        )
+
+    def test_flipped_gate_decomp(self):


Based on my comment about flipped_gate, this test is unnecessary and test_standard_gate_decomp should test all the gates.

this should be fixed in the recent update

jcjaskula-aws · 2023-06-15T15:32:38Z

tests/providers/test_adapter.py

+                    if len(values) == 1:
+                        self.assertTrue(
+                            values[0] < 0.05,
+                            f"Missing {key} key in one of the results.",


The message does not correspond to the test. If the test checks 2 points, let's have 2 tests.

test was unnecessary so I removed it all together since that condition is covered by calculating and reading the percent difference

jcjaskula-aws · 2023-06-15T15:36:59Z

tests/providers/test_adapter.py

+
+                braket_circuit = convert_qiskit_to_braket_circuit(qiskit_circuit)
+
+                task = device.run(braket_circuit, shots=1000)


Simulators can compute the exact waveforms. You can pass shots=0 and you don't have to use eps threshold.

I found that adjusting the adapter so that it yielded only correct states from a given circuit would yield incorrect results when actually simulating the circuit. I chose to prioritize the actual behavior of the circuit as being correct which is what is tested in the updated tests after attempting and looking testing off the stricter state definition. This finding was also corroborated by the subsequent failure of the test_random_circuit test in test_braket_provider when I perfected the state tranformations of the ecr,rzx, and paulievolution gates. The adapter now converts qiskit circuits so that they produce similar results when simulated, but there the state vectors do not coincide when working with ecr,rzx, and paulievolution gates

jcjaskula-aws · 2023-06-19T19:02:06Z

tests/providers/test_braket_backend.py

-                    .result()
-                    .get_counts()
-                )
+                braket_result = backend.run(circuit, shots=1000).result().get_counts()


please comment why you modified this test. I would prefer not to change them if there is no reason.

@jcjaskula-aws I modified this test to not transpile since I viewed it as being redundant to the transpilation which I incorporated into the braket to qiskit converter in the Adapter which is used by the BraketLocalBackend. This also serves to further stress test the circuit transpilation of the braket to qiskit converter by having the converter handle the transpilation.

* [UnitaryHack] Enable qiskit transpilation discontinous qubit indices (qiskit-community#108) * convert Rigetti Aspen qubit connectivity continous * Add qiskit transpilation discountinous qubit indices * Add more comments, cleanup * Add more comments, cleanup * Move in Rigetti branching since not require globally * Fix lint * Add better example * Fix docstring * Add mock for rigetti aspen m3 * Enable transpilation test * Fix mypy * [UnitaryHack] Gate Decomposition(qiskit-community#90) added and tested (qiskit-community#111) * implement and test gate decomposition * Implement and Test Gate Decomposition * addressed comments in previous PR * Adding separate QUEUED and RUNNING states to task status (qiskit-community#110) * fix: Updates for Qiskit to Braket circuit conversion (qiskit-community#97) * Probability to Sample result type * passes tox lint * updates toxlint * fixes observable z error but not passses elint * unit test added * final commit with tests fix: Fallback to JAQCD for device properties (qiskit-community#104) * fix: Fallback to JAQCD for device properties * fix: reformat replacing AWSBraketJob with AmazonBraketTask run the notebooks containing AmazonBraketTask deprecation warning added for AWSBraketJob class formatting fixed re-add <JOB_ARN> fix breaking changes on /provider/__init__.py and __init__.py rename job_id to task_id argument for AmazonBraketTask() rename braket_jobs_states to braket_tasks_states add task_id method to access the task_id similar to the one we have in the JobV1 class adding tests for new class 'AmazonBraketTask' and old class 'AWSBraketJob' formatting test_braket_job.py correcting definition of AWSBraketJob rewriting line 184 in braket_job.py adding class docstring to AWSBraketJob standard import 'from warnings import warn' placed before 'from braket.aws import AwsQuantumTask' (wrong-import-order) docstring fix in test_braket_job.py assert the job id in test_AWS_job() function in test_braket_job.py running how to notebook #0 running how to notebook #1 running how to notebook #2 running how to notebook #3 running how to notebook number 5 running tutorial notebook number 3 tutorial number 0 * add separate QUEUED and RUNNING states to task status (qiskit-community#46) --------- Co-authored-by: Yuri Han <45699207+urihan@users.noreply.github.com> --------- Co-authored-by: WingCode <smallstar1234@gmail.com> Co-authored-by: Gmontes01 <118773601+Gmontes01@users.noreply.github.com> Co-authored-by: Yuri Han <45699207+urihan@users.noreply.github.com>

Gmontes01 and others added 3 commits June 11, 2023 13:47

implement and test gate decomposition

893c1f7

Merge branch 'qiskit-community:main' into main

6233416

Implement and Test Gate Decomposition

71380f2

jcjaskula-aws reviewed Jun 15, 2023

View reviewed changes

addressed comments in previous PR

64d5b8b

jcjaskula-aws reviewed Jun 19, 2023

View reviewed changes

christianbmadsen merged commit 9dd7e2f into qiskit-community:main Jun 19, 2023
4 checks passed

christianbmadsen mentioned this pull request Jun 19, 2023

Gate decomposition #90

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UnitaryHack] Gate Decomposition(#90) added and tested #111

[UnitaryHack] Gate Decomposition(#90) added and tested #111

Gmontes01 commented Jun 14, 2023

jcjaskula-aws Jun 15, 2023

Gmontes01 Jun 19, 2023

jcjaskula-aws Jun 15, 2023

Gmontes01 Jun 19, 2023

jcjaskula-aws Jun 15, 2023

Gmontes01 Jun 17, 2023

Gmontes01 Jun 19, 2023

Gmontes01 Jun 19, 2023

jcjaskula-aws Jun 19, 2023

Gmontes01 Jun 19, 2023

jcjaskula-aws Jun 19, 2023

jcjaskula-aws Jun 21, 2023

Gmontes01 Jun 21, 2023

jcjaskula-aws Jun 21, 2023

jcjaskula-aws Jun 15, 2023

Gmontes01 Jun 15, 2023

Gmontes01 Jun 19, 2023

jcjaskula-aws Jun 19, 2023

jcjaskula-aws Jun 15, 2023

Gmontes01 Jun 19, 2023

jcjaskula-aws Jun 15, 2023

Gmontes01 Jun 19, 2023

jcjaskula-aws Jun 15, 2023

Gmontes01 Jun 19, 2023

jcjaskula-aws Jun 19, 2023

Gmontes01 Jun 19, 2023


		braket_circuit = convert_qiskit_to_braket_circuit(qiskit_circuit)

		task = device.run(braket_circuit, shots=1000)

[UnitaryHack] Gate Decomposition(#90) added and tested #111

[UnitaryHack] Gate Decomposition(#90) added and tested #111

Conversation

Gmontes01 commented Jun 14, 2023

Summary

Details and comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment