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

Support arbitrary exponents when exporting X/Y/ZPowGate to quirk url #2513

Merged
merged 11 commits into from
Nov 19, 2019

Conversation

Strilanc
Copy link
Contributor

@Strilanc Strilanc commented Nov 7, 2019

No description provided.

@Strilanc Strilanc requested a review from viathor November 7, 2019 23:18
@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Nov 7, 2019
Copy link
Collaborator

@viathor viathor left a comment

Choose a reason for hiding this comment

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

This PR doesn't pass automated checks.


if gate.global_shift == -0.5:
return QuirkOp({'id': 'R{d}}ft', 'arg': f'{gate.exponent:.4f}pi'})
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: By comparing to l.138, it seems you may have meant ^ instead of second }, i.e. 'R{d}^ft'.

What's "ft"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the extra }.

ft is an abbreviation of "function of t".

if e is None:
return None
return QuirkOp('X' + e)
def xyz_to_known(axis: str, gate: ops.EigenGate) -> QuirkOp:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional nit: "to_known" is weird, because it lacks a noun. How about "xyz_to_quirk_op"? Any ideas for better names for these functions?

(Applies to other functions below, too.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Strilanc Strilanc added the Ready for Re-Review For when reviewers take their time. label Nov 16, 2019
Copy link
Collaborator

@viathor viathor left a comment

Choose a reason for hiding this comment

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

LGTM, only minor and optional comments.

if e is None:
return None
return QuirkOp('X' + e)
def xyz_to_quirk_op(axis: str, gate: ops.EigenGate) -> QuirkOp:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: Signature of this function makes it appear that it is appropriate to use it for general EigenGates, not just Pauli-PowGates. It's easy to remedy this by taking global_shift and exponent as arguments in place of the gate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged, but this is a private method not exposed to users.

@@ -54,9 +57,35 @@ def same_half_turns(a1: float, a2: float, atol=0.0001) -> bool:
return abs(d) < atol


def angle_to_exponent_key(t: Union[float, sympy.Symbol]) -> Optional[str]:
def _is_supported_formula(formula: sympy.Basic):
Copy link
Collaborator

Choose a reason for hiding this comment

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

-> bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return False


def _val_to_quirk_formula(t: Union[float, sympy.Basic]) -> Optional[str]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

-> str

(None is never returned.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


def _val_to_quirk_formula(t: Union[float, sympy.Basic]) -> Optional[str]:
if isinstance(t, sympy.Basic):
if not set(t.free_symbols) <= {sympy.Symbol('t')}:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: This conditional is part of our validation logic, so it fits better in _is_supported_formula. I understand you've placed it here to generate more informative exceptions. An alternative that gives you both is a function _check_formula_supported that doesn't return anything but raises an exception on unsupported formulas. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged.

@@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import numpy as np
Copy link
Collaborator

Choose a reason for hiding this comment

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

pylint wants this gone

@Strilanc Strilanc added automerge Tells CirqBot to sync and merge this PR. (If it's running.) and removed Ready for Re-Review For when reviewers take their time. labels Nov 18, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 18, 2019
@CirqBot
Copy link
Collaborator

CirqBot commented Nov 18, 2019

Automerge cancelled: A status check is failing.

@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 Nov 18, 2019
@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 19, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 19, 2019
@CirqBot CirqBot merged commit f14ed20 into master Nov 19, 2019
@CirqBot CirqBot deleted the quirk_toolingG branch November 19, 2019 23:39
@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 Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants