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

Support ActOnStabilizerCHFormArgs in the common gates and testing #3203

Merged
merged 24 commits into from
Oct 28, 2020
Merged

Support ActOnStabilizerCHFormArgs in the common gates and testing #3203

merged 24 commits into from
Oct 28, 2020

Conversation

smitsanghavi
Copy link
Collaborator

Also some minor bug fixes and refactoring in StabilizerStateCHForm, ActOnCliffordTableauArgs and consistent_act_on.

#2948 #2423

@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Aug 10, 2020
@balopat balopat self-assigned this Aug 13, 2020
@smitsanghavi smitsanghavi added the Ready for Re-Review For when reviewers take their time. label Aug 18, 2020
balopat
balopat suggested changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

Hey @smitsanghavi, thanks for submitting this! I started reviewing it! I will need to dig deeper in the next couple of days but I found some nits to cleanup in the meantime.

cirq/sim/clifford/act_on_stabilizer_ch_form_args_test.py Outdated Show resolved Hide resolved
cirq/ops/common_gates.py Outdated Show resolved Hide resolved
cirq/sim/clifford/act_on_stabilizer_ch_form_args.py Outdated Show resolved Hide resolved
cirq/sim/clifford/act_on_stabilizer_ch_form_args.py Outdated Show resolved Hide resolved
cirq/testing/consistent_act_on.py Outdated Show resolved Hide resolved
cirq/testing/consistent_act_on.py Outdated Show resolved Hide resolved
cirq/sim/clifford/act_on_clifford_tableau_args.py Outdated Show resolved Hide resolved
cirq/sim/clifford/act_on_stabilizer_ch_form_args.py Outdated Show resolved Hide resolved
@balopat balopat removed the Ready for Re-Review For when reviewers take their time. label Aug 19, 2020
@smitsanghavi smitsanghavi mentioned this pull request Sep 16, 2020
Merged
Strilanc
Strilanc reviewed Sep 23, 2020
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 Outdated Show resolved Hide resolved
@smitsanghavi smitsanghavi changed the title Add ActOnStabilizerCHFormArgs. Support it in the common gates and testing Support ActOnStabilizerCHFormArgs in the common gates and testing Sep 28, 2020
@smitsanghavi smitsanghavi added the Ready for Re-Review For when reviewers take their time. label Sep 28, 2020
Strilanc
Strilanc approved these changes Oct 21, 2020
View reviewed changes
cirq/ops/common_gates.py Outdated Show resolved Hide resolved
cirq/ops/common_gates.py
assert all(
gate._act_on_(args) # type: ignore
for gate in [ZPowGate(), HPowGate()])
state.omega *= (1 + 1j) / (2**0.5) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the type ignore needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mypy assumes state.omega is an int since it was initialized by an int. It complains when I try to assign a complex value to it. Maybe adding + 0j to the initialization will fix it. Let me know if you think that's better.

balopat
balopat suggested changes Oct 21, 2020
View reviewed changes
Copy link
Contributor

@balopat balopat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left a bunch of comments!
Also, I'm a bit worried about the fact that phases just go unnoticed...
In assert_all_implemented_act_on_effects_match_unitary shouldn't we use

      np.testing.assert_allclose(np.reshape(
            stabilizer_ch_form.state_vector(), protocols.qid_shape(qubits)),
                                                   state_vector,
                                                   atol=1e-07, err_msg="inconsistent stabilizer ch form")

instead of

        testing.assert_allclose_up_to_global_phase(np.reshape(
            stabilizer_ch_form.state_vector(), protocols.qid_shape(qubits)),
                                                   state_vector,
                                                   atol=1e-07)

?
One of the benefits of the CH-form is that it is phase sensitive. Why are we checking unitary consistency in a phase insensitive way?

cirq/ops/common_gates_test.py Outdated Show resolved Hide resolved
cirq/testing/consistent_act_on.py Show resolved Hide resolved
cirq/testing/consistent_act_on.py Outdated Show resolved Hide resolved
cirq/ops/common_gates.py Outdated Show resolved Hide resolved
cirq/ops/common_gates.py Outdated Show resolved Hide resolved
cirq/ops/common_gates_test.py Outdated Show resolved Hide resolved
@balopat balopat removed the Ready for Re-Review For when reviewers take their time. label Oct 21, 2020
@smitsanghavi
Copy link
Collaborator Author

Left a bunch of comments!
Also, I'm a bit worried about the fact that phases just go unnoticed...
In assert_all_implemented_act_on_effects_match_unitary shouldn't we use

      np.testing.assert_allclose(np.reshape(
            stabilizer_ch_form.state_vector(), protocols.qid_shape(qubits)),
                                                   state_vector,
                                                   atol=1e-07, err_msg="inconsistent stabilizer ch form")

instead of

        testing.assert_allclose_up_to_global_phase(np.reshape(
            stabilizer_ch_form.state_vector(), protocols.qid_shape(qubits)),
                                                   state_vector,
                                                   atol=1e-07)

?
One of the benefits of the CH-form is that it is phase sensitive. Why are we checking unitary consistency in a phase insensitive way?

That makes sense. There were some tests adding extra global phases that were breaking it previously. Added support for global_shift to fix this.

@smitsanghavi smitsanghavi requested a review from balopat October 26, 2020 07:12
@smitsanghavi smitsanghavi added the Ready for Re-Review For when reviewers take their time. label Oct 26, 2020
balopat
balopat approved these changes Oct 27, 2020
View reviewed changes
Copy link
Contributor

@balopat balopat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thank you for all the hard work on this!!! Can you just address the two last nits? Thank you!

@balopat balopat 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 Oct 27, 2020
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Oct 27, 2020
@balopat balopat 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 Oct 27, 2020
@smitsanghavi smitsanghavi merged commit e4c191d into quantumlib:master Oct 28, 2020
@smitsanghavi smitsanghavi deleted the actch branch October 28, 2020 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@balopat balopat balopat approved these changes

@Strilanc Strilanc Strilanc approved these changes

Assignees

@balopat balopat

Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@smitsanghavi @Strilanc @balopat @googlebot @CirqBot