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

Implemented 8n T complexity decomposition of LessThanEqual gate #6156

Merged
merged 17 commits into from Jul 17, 2023

Conversation

NoureldinYosri
Copy link
Collaborator

@CirqBot CirqBot added the size: M 50< lines changed <250 label Jun 22, 2023
@NoureldinYosri
Copy link
Collaborator Author

@mpharrigan this is now ready for review


@babbush @Strilanc fyi this is an implementation of your design.

cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
if context is None:
context = cirq.DecompositionContext(cirq.ops.SimpleQubitManager())

P, Q, target = (qubits[: self.x_bitsize], qubits[self.x_bitsize : -1], qubits[-1])
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use pep8 variable names here

why are we not using GateWithRegisters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this gate was created as a cirq.ArithmeticGate, should I change that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It can be done later, but I think would work nicely here since you manually have to segregate the qubits into registers during decomposte at present.

cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
@NoureldinYosri
Copy link
Collaborator Author

this is ready for another look @mpharrigan

cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
Comment on lines 312 to 319
if d == 0:
return infra.TComplexity(t=8 * n - 4, clifford=46 * n - 17)
elif d == 1:
return infra.TComplexity(t=8 * n, clifford=46 * n + 3 + 2 * is_second_longer)
else:
return infra.TComplexity(
t=8 * n + 4 * d - 4, clifford=46 * n + 17 * d - 14 + 2 * is_second_longer
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

bump

Comment on lines +321 to +322
def _has_unitary_(self):
return True
Copy link
Collaborator

Choose a reason for hiding this comment

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

bump

Comment on lines +38 to +40
# TODO: Do not decompose {cq.And, cq.LessThanEqualGate, cq.LessThanGate} because the
# `cq.map_clean_and_borrowable_qubits` currently gets confused and is not able to re-map qubits
# optimally; which results in a higher number of ancillas thus the tests fails due to OOO.
Copy link
Collaborator

Choose a reason for hiding this comment

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

bump

@NoureldinYosri
Copy link
Collaborator Author

@mpharrigan this is ready for another look


from cirq._compat import cached_property
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: why can't we use functools.cached_property in cirq?

Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

Fix all the links to the referenced paper, but LGTM

@@ -130,6 +131,147 @@ def _t_complexity_(self) -> infra.TComplexity:
)


@attr.frozen
class BiQubitsMixer(infra.GateWithRegisters):
"""Implements the COMPARE2 (Fig. 1) https://www.nature.com/articles/s41534-018-0071-5#Sec8
Copy link
Collaborator

Choose a reason for hiding this comment

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

this links to the acknowledgements section lol. Please remove the #Sec8.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sec8 is the "Electronic supplementary material" which is where the decomposition is. it's not part of the main paper but is in the supplementary material

cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Show resolved Hide resolved
cirq-ft/cirq_ft/algos/arithmetic_gates.py Outdated Show resolved Hide resolved
@NoureldinYosri NoureldinYosri merged commit c93224e into master Jul 17, 2023
30 checks passed
@mpharrigan mpharrigan added the area/cirq-ft Issues related to the Cirq-FT sub-package label Jul 26, 2023
@NoureldinYosri NoureldinYosri deleted the LessThanOrEqual branch October 26, 2023 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cirq-ft Issues related to the Cirq-FT sub-package size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants