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

Add (de)serializer for PhasedXZGate #2598

Merged
merged 4 commits into from
Nov 27, 2019
Merged

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented Nov 25, 2019

No description provided.

@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Nov 25, 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 with a few minor issues.

cirq.testing.assert_allclose_up_to_global_phase(
cirq.unitary(deserialized_op),
cirq.unitary(op),
atol=1e-7,
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the input values you've chosen can be represented as floats exactly, so I wonder whether we could test this for exact equality?

I'm concerned that we may be missing an issue where a float is serialized as string with insufficient number of characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many tests in this module have the same issue, in that we are comparing serialized representations but we have to pick certain values to get exact equality of floats. I agree this is something we should solve, but perhaps we can do that in a separate PR for the entire module, rather than a one-off for this new case. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this is definitely out of the scope of this PR.

FTR from our offline discussion: serialization and deserialization of values such as 0.5 or 0.125 that are represented exactly in both single and double precision floats should yield result exactly equal to the original value. We should not be throwing our hands up just because floats ;-)

)



Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: let's make it two empty lines for consistency

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.

@@ -127,6 +127,54 @@ def _near_mod_2(e, t, atol=1e-8):
]


#
# PhasedXZ gate (de)serializer
# TODO(maffoo): Move to SINGLE_QUBIT_(DE)SERIALIZERS when server support is in.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to file an issue and reference it in place of your username.

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: #2602.

Copy link
Collaborator

@dstrain115 dstrain115 left a comment

Choose a reason for hiding this comment

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

LGTM

@maffoo maffoo added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 27, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 27, 2019
@CirqBot CirqBot merged commit 912c82a into master Nov 27, 2019
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 27, 2019
@CirqBot CirqBot deleted the u/maffoo/phasedxz-serialize branch November 27, 2019 18:41
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 27, 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.

None yet

5 participants