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.

Sign up for GitHub

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

Jump to bottom

Add Rx, Ry, Rz gates #3920

Merged
merged 7 commits into from
Mar 18, 2021
Merged

Add Rx, Ry, Rz gates #3920

merged 7 commits into from
Mar 18, 2021

Conversation

tanujkhattar
Copy link
Collaborator

This PR creates separate Rx, Ry, Rz gates.

@tanujkhattar tanujkhattar requested review from cduck, vtomole and a team as code owners March 16, 2021 02:20
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Mar 16, 2021
@tanujkhattar tanujkhattar changed the title WIP: Add Rx, Ry, Rz gates Add Rx, Ry, Rz gates Mar 16, 2021
@tanujkhattar tanujkhattar linked an issue Mar 16, 2021 that may be closed by this pull request
Compiling: Separate R* and *Pow gates #3640
Closed
maffoo
maffoo reviewed Mar 16, 2021
View reviewed changes
cirq/ops/common_gates.py Outdated Show resolved Hide resolved
cirq/ops/common_gates.py Outdated Show resolved Hide resolved
cirq/ops/common_gates.py Show resolved Hide resolved
cirq/ops/common_gates.py
return protocols.CircuitDiagramInfo(
wire_symbols=('X',), exponent=self._diagram_exponent(args)
)

def _qasm_(self, args: 'cirq.QasmArgs', qubits: Tuple['cirq.Qid', ...]) -> Optional[str]:
args.validate_version('2.0')
if self._exponent == 1:
if self._exponent == 1 and self._global_shift != -0.5:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? The other case is ignoring global phase anyway, so it's not clear why we should be picky here.

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 is to fix (at least partially) #1800.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Thanks.

maffoo
maffoo approved these changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@maffoo maffoo left a comment

Choose a reason for hiding this comment

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

LGTM. Might be good to get another maintainer to look it over as well, since these are such important gates, but up to you.

@tanujkhattar tanujkhattar mentioned this pull request Mar 17, 2021
Closed
balopat
balopat reviewed Mar 17, 2021
View reviewed changes
cirq/ops/__init__.py
@@ -58,6 +58,9 @@
CZPowGate,
H,
HPowGate,
Rx,
Ry,
Rz,
rx,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can deprecate these in favor for the R* constructors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Filed an issue to track this: #3929

@maffoo
Copy link
Contributor

maffoo commented Mar 17, 2021

Discussed in cirq sync: we should implement __pow__ on these R* gates so they stay as rotation gates when exponentiating.

@tanujkhattar
Copy link
Collaborator Author

@maffoo Exponentiating R* gates work as expected because _with_exponent is already overloaded. Added explicit tests for the same.

@tanujkhattar
Copy link
Collaborator Author

@balopat Do you want to take another look or should I merge?

@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Mar 18, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Mar 18, 2021
@CirqBot CirqBot merged commit fa55cbb into quantumlib:master Mar 18, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Mar 18, 2021
@mpharrigan mpharrigan added the BREAKING CHANGE For pull requests that are important to mention in release notes. label Apr 19, 2021
@mpharrigan
Copy link
Collaborator

This breaks pytket which uses type(obj) to look up gate types instead of doing an isinstance() check. Ironically the pytket name for their rotation gate it Rx etc. but they are only looking for the PowGates in cirq circuits. (pytket 0.7)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maffoo maffoo maffoo approved these changes

@balopat balopat balopat approved these changes

@cduck cduck Awaiting requested review from cduck

@vtomole vtomole Awaiting requested review from vtomole

Assignees
No one assigned
Labels
BREAKING CHANGE For pull requests that are important to mention in release notes. cla: yes Makes googlebot stop complaining.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compiling: Separate R* and *Pow gates
5 participants
@tanujkhattar @maffoo @mpharrigan @balopat @CirqBot