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

Make SingleQubitCompare output registers directional #6312

Merged

Conversation

tanujkhattar
Copy link
Collaborator

@tanujkhattar tanujkhattar requested a review from fdmalone October 9, 2023 22:15
@tanujkhattar tanujkhattar requested review from vtomole, cduck and a team as code owners October 9, 2023 22:15
@CirqBot CirqBot added the size: S 10< lines changed <50 label Oct 9, 2023
@tanujkhattar tanujkhattar added the area/cirq-ft Issues related to the Cirq-FT sub-package label Oct 9, 2023
@@ -222,7 +222,15 @@ class SingleQubitCompare(infra.GateWithRegisters):

@cached_property
def signature(self) -> infra.Signature:
return infra.Signature.build(a=1, b=1, less_than=1, greater_than=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should BiQubitsMixer be updated too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so, but I'd recommend to open a separate issue in Qualtran. I haven't looked at the decomposition too closely, but I'm not sure if ancilla doesn't register doesn't need to be part of the signature at all or whether it should be a directional register like the target of an And gate.

Maybe @NoureldinYosri can clarify this on the separate issue and then we can send a PR to update it and it's usage. Also, it looks like we are not really using BiQubitsMixer(adjoint=True) anywhere.

Copy link
Contributor

@fdmalone fdmalone Oct 9, 2023

Choose a reason for hiding this comment

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

Ok. I'll bump my cirq version locally and see. This is good to merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

the Adjoint=True is used during the decomposition I believe.

Screenshot 2023-10-09 at 4 13 04 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tanujkhattar @fdmalone it should be directional like And

@fdmalone fdmalone enabled auto-merge (squash) October 9, 2023 22:36
@fdmalone fdmalone merged commit fefe350 into quantumlib:master Oct 9, 2023
33 checks passed
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: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bloq counts fails for LessThanGate / SingleQubitCompare decomposition
4 participants