[CQT-43] Take global phases into account when verifying decompositions #368

juanboschero · 2024-10-28T16:39:31Z

Changed decomposer object logic
Phase of qubits are now tracked by circuit object
Tests added to verify and correct global phase leak into relative phase

rturrado

Nice work!

I would mainly have a look at the check_gate_replacements function.

I would also try to:

Encapsulate the phase map in a class.
Use more functions.
Make the code more readable.

rturrado · 2024-11-06T11:32:59Z

opensquirrel/circuit.py

@@ -101,6 +106,22 @@ def qubit_register_name(self) -> str:
    def bit_register_name(self) -> str:
        return self.register_manager.get_bit_register_name()

+    def _set_phase_map(self, phase_map: NDArray[np.complex128]) -> None:


Can we have a PhaseMap class?
The description of the class should explain what are the keys and values of that map.
The implementation of that class should hide that we are internally using an NDArray.

rturrado · 2024-11-06T11:35:00Z

opensquirrel/circuit.py

+
+        return np.complex128(self.qubit_phase_map[Qubit(qubit).index])
+
+    def add_qubit_phase(self, qubit: QubitLike, phase: np.complex128) -> ArrayLike:


Why is this function returning something? It should just update a qubit phase.

rturrado · 2024-11-06T11:35:34Z

opensquirrel/circuit.py

+        temp_phase_map = self.qubit_phase_map
+        temp_phase_map[Qubit(qubit).index] += phase
+        self.qubit_phase_map = temp_phase_map
+        return temp_phase_map


We shouldn't need this temporary. A user can update a qubit phase and then get a qubit phase.

rturrado · 2024-11-06T11:37:42Z

opensquirrel/common.py


-    phase_difference = matrix_a[first_non_zero] / matrix_b[first_non_zero]
+    return np.complex128(matrix_a[first_non_zero] / matrix_b[first_non_zero])



opensquirrel/decomposer/general_decomposer.py

rturrado · 2024-11-06T11:52:41Z

opensquirrel/decomposer/general_decomposer.py

+        if len(gate_qubit_indices) > 1:
+            relative_phase = float(np.real(qc.get_qubit_phase(qubit_list[1]) - qc.get_qubit_phase(qubit_list[0])))


Code should be more readable. For example:

if (circuit_has_two_qubits): relative_phase = get_relative_phase(qc)

All that float(blah - blah) deserves a function on its own. Why not calling qc.get_relative_phase()?

What happens if the circuit has 3 or more qubits?

rturrado · 2024-11-06T11:55:48Z

opensquirrel/decomposer/general_decomposer.py


-def decompose(ir: IR, decomposer: Decomposer) -> None:
+def decompose(circuit: Circuit, decomposer: Decomposer) -> None:


If Circuit cannot be None here, it shouldn't be None at check_gate_replacement either. That would make Circuit the first parameter at check_gate_replacement also. Anyway, I think the global phase work shouldn't be part of check_gate_replacement. Once we have the replacement_gates, line 86, we could go further and check_global_phase_correction or something like that.

Moreover, why not delaying the check_global_phase_correction out of the decomposition? Is this because that Rz instruction that can be added at the end may need to be decomposed in some cases?

test/test_integration.py

rturrado · 2024-11-06T12:02:19Z

test/test_integration.py

@@ -115,6 +115,81 @@ def test_Spin2_backend() -> None:
    )


+def test_integration_global_phase() -> None:


Can we add a comment somewhere in the code saying where a global phase is expected to be introduced?

rturrado · 2024-11-06T12:03:24Z

test/test_integration.py

+qubit[3] q
+
+Rz(1.5707963) q[1]
+X90 q[1]
+Rz(1.5707963) q[1]
+Rz(3.1415927) q[0]
+X90 q[0]
+Rz(0.0081036221) q[0]
+X90 q[0]
+Rz(3.1415927) q[0]
+CZ q[1], q[0]
+X90 q[2]
+Rz(1.3907963) q[2]
+X90 q[2]
+X90 q[0]
+Rz(0.051592695) q[0]
+X90 q[0]
+Rz(3.1415927) q[0]
+CZ q[2], q[0]
+Rz(1.5707963) q[0]
+X90 q[0]
+Rz(1.5707963) q[0]
+Rz(3.1415927) q[1]
+X90 q[1]
+Rz(2.8235927) q[1]
+X90 q[1]


Just out of curiosity, what instructions, if any, do the global phase correction here?

Co-authored-by: Roberto Turrado Camblor <[email protected]>

rturrado · 2024-11-18T10:20:38Z

opensquirrel/circuit.py

+        if qubit in self.phase_map:
+            return True
+
+        return False


Suggested change

if qubit in self.phase_map:

return True

return False

return qubit in self.phase_map

rturrado · 2024-11-18T10:22:41Z

opensquirrel/circuit.py

+    def set_phase_map(self, phase_map: NDArray[np.complex128]) -> None:
+        self.qubit_phase_map = phase_map
+


Suggested change

def set_phase_map(self, phase_map: NDArray[np.complex128]) -> None:

self.qubit_phase_map = phase_map

I think this is a copy construction, and should be taken care of in the/a constructor.

elenbaasc

This still needs some looking into.

I think the phase map may be useful (necessary), but I'm not convinced yet.

elenbaasc · 2024-11-18T14:49:39Z

opensquirrel/circuit.py

+
+
+@dataclass(init=False)
+class PhaseMap(Circuit):


This is quite an odd construction: a dataclass that subclasses from a normal class. Also, why would a PhaseMap have a 'is a' relationship with a Circuit, i.e., it is not clear why a PhaseMap would subclass from a Circuit (just like it is not clear why a Parser 'is a' InstructionLibrary)?

It's not that it's not clear. It's wrong. It's a wrong way to get access to information from the base class. But inheritance should only be used when there is an is a relationship with the base class.

A PhaseMap should be an independent data structure, managing some data, e.g., a map of phases, encapsulating the definition of that map of phases (today a dictionary, tomorrow whatever), and providing a given API to access, query, etc. that data.

If PhaseMap turns out to be very simple, e.g., just a map over which we only do set and get, we can decide not to use it. But at the very first moment that it is more complex than that, you won't want to be writing complicating code all around the code base to access a map of phases. In that case, we'd better have a PhaseMap.

elenbaasc · 2024-11-18T14:54:59Z

opensquirrel/circuit.py

+    def __init__(self, phase_map: ArrayLike[np.complex128]):
+        """Initialize a PhaseMap object."""
+        self.phase_map = phase_map


This seems unnecessary for a 'dataclass', normally if the properties of the dataclass need to be preprocessed, one would use the __post_init__ method. But since here the property is not processed at all, I'm struggling to see why an __init__ is needed at all...

elenbaasc · 2024-11-18T14:56:19Z

opensquirrel/circuit.py

+    def set_phase_map(self, phase_map: NDArray[np.complex128]) -> None:
+        self.qubit_phase_map = phase_map


Setter and getter functionality is sort of built-in in dataclasses, that's there purpose. So this is either not needed, or this should be a normal class, where the phase_map is a property.

elenbaasc · 2024-11-19T08:27:16Z

opensquirrel/common.py

+def to_euler_form(scalar: np.complex128) -> np.complex128:
+    """ " Derives the Euler rotation angle from a scalar.


Suggested change

def to_euler_form(scalar: np.complex128) -> np.complex128:

""" " Derives the Euler rotation angle from a scalar.

def get_phase_angle(scalar: np.complex128) -> np.complex128:

"""Derives the Euler phase angle from a scalar.

elenbaasc · 2024-11-19T09:43:46Z

opensquirrel/decomposer/general_decomposer.py

@@ -32,25 +55,37 @@ def check_gate_replacement(gate: Gate, replacement_gates: Iterable[Gate]) -> Non
        msg = f"replacement for gate {gate.name} does not preserve the quantum state"
        raise ValueError(msg)

+    if qc is not None:


I guess qc should be circuit, but then the methods that are called on circuit are the methods of the PhaseMap class, i.e., not of the Circuit class.

I'm still confused what the code block (of this if-statement) actually does. I get the idea, but I feel that this might actually introduce relative phases, instead of mitigating them...

Can you better explain in the Confluence page what is being implemented and how that prevents a global phase difference in the gates (between original and replacement), from resulting in a relative phase between the qubits, because that bit is still unclear to me. To put it another way: How can a global phase difference in a gate replacement, lead to a relative phase difference. As far as I see it, this can only happen if a replaced gate (that differs by a global phase from the original) is made into a controlled gate, but this doesn't happen anywhere in the compilation process (yet).

Juan Boschero added 3 commits October 28, 2024 16:27

init

10b2879

Move to circuit

b267033

Fix pytest and formatting errors

de8f9f3

juanboschero requested review from rturrado and elenbaasc November 5, 2024 14:59

rturrado changed the title ~~[CQT 43] take global phases into account when verifying decompositions~~ [CQT 43] Take global phases into account when verifying decompositions Nov 6, 2024

rturrado suggested changes Nov 6, 2024

View reviewed changes

juanboschero and others added 2 commits November 8, 2024 11:25

Apply suggestions from code review

ed78386

Co-authored-by: Roberto Turrado Camblor <[email protected]>

init

df085e1

rturrado suggested changes Nov 18, 2024

View reviewed changes

elenbaasc reviewed Nov 19, 2024

View reviewed changes

rturrado changed the title ~~[CQT 43] Take global phases into account when verifying decompositions~~ [CQT-43] Take global phases into account when verifying decompositions Nov 19, 2024

Merge from develop

b5fad34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CQT-43] Take global phases into account when verifying decompositions #368

[CQT-43] Take global phases into account when verifying decompositions #368

juanboschero commented Oct 28, 2024

rturrado left a comment

rturrado Nov 6, 2024

rturrado Nov 6, 2024

rturrado Nov 6, 2024

rturrado Nov 6, 2024

rturrado Nov 6, 2024

rturrado Nov 6, 2024

rturrado Nov 6, 2024

rturrado Nov 6, 2024

rturrado Nov 6, 2024

rturrado Nov 6, 2024

rturrado Nov 18, 2024

rturrado Nov 18, 2024

elenbaasc left a comment

elenbaasc Nov 18, 2024

rturrado Nov 19, 2024

elenbaasc Nov 18, 2024

elenbaasc Nov 18, 2024

elenbaasc Nov 19, 2024

elenbaasc Nov 19, 2024


		return np.complex128(self.qubit_phase_map[Qubit(qubit).index])

		def add_qubit_phase(self, qubit: QubitLike, phase: np.complex128) -> ArrayLike:


		phase_difference = matrix_a[first_non_zero] / matrix_b[first_non_zero]
		return np.complex128(matrix_a[first_non_zero] / matrix_b[first_non_zero])

		if len(gate_qubit_indices) > 1:
		relative_phase = float(np.real(qc.get_qubit_phase(qubit_list[1]) - qc.get_qubit_phase(qubit_list[0])))


		def decompose(ir: IR, decomposer: Decomposer) -> None:
		def decompose(circuit: Circuit, decomposer: Decomposer) -> None:

		@@ -115,6 +115,81 @@ def test_Spin2_backend() -> None:
		)


		def test_integration_global_phase() -> None:

		def set_phase_map(self, phase_map: NDArray[np.complex128]) -> None:
		self.qubit_phase_map = phase_map

		def to_euler_form(scalar: np.complex128) -> np.complex128:
		""" " Derives the Euler rotation angle from a scalar.

[CQT-43] Take global phases into account when verifying decompositions #368

Are you sure you want to change the base?

[CQT-43] Take global phases into account when verifying decompositions #368

Conversation

juanboschero commented Oct 28, 2024

rturrado left a 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

elenbaasc left a 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