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

feat: Improved handling of controlled gates in converters #391

Merged
merged 34 commits into from
Sep 24, 2024

Conversation

CalMacCQ
Copy link
Contributor

@CalMacCQ CalMacCQ commented Sep 12, 2024

Description

This PR adds support for handling the control_state of a controlled operation in the qiskit_to_tk and tk_to_qiskit converters. The control state is now handled directly instead of adding X gates on the control qubits.

I've also taken the liberty of refactoring out the "QControlBox building" from the CircuitBuildier.add_qiskit_data method in the same spirit as #382

I've also reworked some internals of how unitary gates are converted to and fro. Handling of controlled unitary boxes was added in #372 . I think it'd be fairly easy to add the other direction into this PR too (see #378) (I think its best to do this separately).

Original PR adding control state handling with X gates -> #118

Related issues

#378
#313
closes #178

Checklist

  • I have performed a self-review of my code.
  • I have commented hard-to-understand parts of my code.
  • I have made corresponding changes to the public API documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the changelog with any user-facing changes.

@CalMacCQ CalMacCQ requested a review from cqc-melf as a code owner September 12, 2024 15:35
@CalMacCQ CalMacCQ marked this pull request as draft September 12, 2024 15:35
@CalMacCQ CalMacCQ changed the title feat: directly handle control state in conversion of controlled gates feat: handle control state in conversion of controlled gates Sep 12, 2024
else:
base_tket_gate: OpType = _known_qiskit_gate[c_gate.base_gate.base_class]

base_op: Op = Op.create(base_tket_gate, params) # type: ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I can get around this type: ignore. Mypy doesn't seem happy with the fact that base_op is defined on two branches.

The _get_unitary_box function has return type Unitary1qBox | Unitary2qBox | Unitary3qBox. Not sure if mypy is able to find out that these unitary boxes inherit from Op.

@CalMacCQ CalMacCQ changed the title feat: handle control state in conversion of controlled gates feat: Improved handling of controlled gates in converters Sep 17, 2024
@@ -865,6 +867,21 @@ def test_controlled_unitary_conversion() -> None:
assert np.allclose(u_qc, u_tkc)


def test_qcontrol_box_conversion_to_qiskit() -> 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.

I think I will add tests for a wider range of controlled operations

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes please.

Copy link
Contributor Author

@CalMacCQ CalMacCQ Sep 18, 2024

Choose a reason for hiding this comment

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

Conversion doesn't work properly for various parameterized gates. Will update the handling and add tests.

Copy link
Contributor Author

@CalMacCQ CalMacCQ Sep 23, 2024

Choose a reason for hiding this comment

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

I've fixed the conversion of parametrised gates in b42946e and added some controlled parametrised gates to the test in c1b315c

@CalMacCQ CalMacCQ marked this pull request as ready for review September 17, 2024 15:18
@CalMacCQ CalMacCQ requested a review from cqc-alec September 17, 2024 15:19
tests/qiskit_convert_test.py Outdated Show resolved Hide resolved
pytket/extensions/qiskit/qiskit_convert.py Show resolved Hide resolved
@@ -865,6 +867,21 @@ def test_controlled_unitary_conversion() -> None:
assert np.allclose(u_qc, u_tkc)


def test_qcontrol_box_conversion_to_qiskit() -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes please.

@CalMacCQ CalMacCQ removed the request for review from cqc-melf September 18, 2024 09:12
DecomposeBoxes().apply(circ1)
DecomposeBoxes().apply(circ2)
assert circ1 == circ2
assert compare_unitaries(circ1.get_unitary(), circ2.get_unitary())
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 think this check of unitary equivalence may be redundant given the previous line

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, can remove this. However, I think we should add a test that compares the unitary of the tket circuit with the unitary of the qiskit circuit (adjusted for endianness).

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 in 05dd936

@CalMacCQ CalMacCQ requested a review from cqc-alec September 23, 2024 12:43
DecomposeBoxes().apply(circ1)
DecomposeBoxes().apply(circ2)
assert circ1 == circ2
assert compare_unitaries(circ1.get_unitary(), circ2.get_unitary())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, can remove this. However, I think we should add a test that compares the unitary of the tket circuit with the unitary of the qiskit circuit (adjusted for endianness).

@CalMacCQ CalMacCQ merged commit e148bd8 into main Sep 24, 2024
6 checks passed
@CalMacCQ CalMacCQ deleted the feat/improved_controlled_gates branch October 24, 2024 21:57
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.

Use improved QControlBox to handle control states in qiskit_to_tk
2 participants