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

change type annotation from float to TParamVal for theta and phi #4266

Closed
wants to merge 3 commits into from
Closed

change type annotation from float to TParamVal for theta and phi #4266

wants to merge 3 commits into from

Conversation

stubbi
Copy link
Contributor

@stubbi stubbi commented Jun 27, 2021 •
edited

resolves #4244

@stubbi stubbi requested review from cduck, vtomole, wcourtney and a team as code owners June 27, 2021 14:39
@stubbi stubbi requested a review from dstrain115 June 27, 2021 14:39
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Jun 27, 2021
@stubbi stubbi mentioned this pull request Jun 27, 2021
Closed
maffoo
maffoo reviewed Jun 29, 2021
View reviewed changes
cirq-google/cirq_google/calibration/workflow_test.py
@@ -1181,7 +1181,7 @@ def test_run_floquet_characterization_for_moments():
itertools.product([0.1, 0.7], [-0.3, 0.1, 0.5], [-0.3, 0.2, 0.4], [-0.6, 0.1, 0.6], [0.2, 0.6]),
)
def test_fsim_phase_corrections(
theta: float, zeta: float, chi: float, gamma: float, phi: float
theta: cirq.value.TParamVal, zeta: float, chi: float, gamma: float, phi: cirq.value.TParamVal
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 this is available on the main cirq namespace, so can just do

Suggested change
theta: cirq.value.TParamVal, zeta: float, chi: float, gamma: float, phi: cirq.value.TParamVal
theta: cirq.TParamVal, zeta: float, chi: float, gamma: float, phi: cirq.TParamVal

Can do this throughout cirq_google, since it's external to cirq_core.

dstrain115
dstrain115 approved these changes Jun 30, 2021
View reviewed changes
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.

Agree with maffoo's comments. Otherwise, LGTM.

@stubbi
Copy link
Contributor Author

stubbi commented Jul 1, 2021

Thanks for the feedback! :-)

Will make the requested changes

tanujkhattar
tanujkhattar requested changes Jul 1, 2021
View reviewed changes
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

In general, I think we should check whether a function whose parameter type is getting changed actually accepts symbols or not. One way of doing this could be to ensure that there's a test with symbols for the function whose parameter type we are changing.

cirq-google/cirq_google/calibration/workflow_test.py
@@ -1181,7 +1181,7 @@ def test_run_floquet_characterization_for_moments():
itertools.product([0.1, 0.7], [-0.3, 0.1, 0.5], [-0.3, 0.2, 0.4], [-0.6, 0.1, 0.6], [0.2, 0.6]),
)
def test_fsim_phase_corrections(
theta: float, zeta: float, chi: float, gamma: float, phi: float
theta: cirq.value.TParamVal, zeta: float, chi: float, gamma: float, phi: cirq.value.TParamVal
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see a reason why only theta / phi should accept TParamVal here. It's not needed for both since we compute the unitary of the expected_gate in the function and hence not using any parameters. Also, it doesn't look consistent with other parameters (zeta, chi, gamma).

cirq-core/cirq/ops/diagonal_gate.py
@@ -144,7 +144,7 @@ def _value_equality_values_(self) -> Any:
return tuple(self._diag_angles_radians)

def _decompose_for_basis(
self, index: int, bit_flip: int, theta: float, qubits: Sequence['cirq.Qid']
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should make the changes here since below _decompose_ explicitly checks that self should not be parameterized and hence the parameter will always be a float here.

@tanujkhattar
Copy link
Collaborator

@stubbi Gentle reminder. Please let me know once the comments are resolved and the PR is ready for re-review.

@CirqBot CirqBot added size: XL lines changed >1000 size: S 10< lines changed <50 and removed size: XL lines changed >1000 labels Aug 12, 2021
@tanujkhattar
Copy link
Collaborator

@stubbi Are you still planning to work on this ?

@tanujkhattar
Copy link
Collaborator

@stubbi Gentle reminder.

@Tinkidinki Tinkidinki mentioned this pull request Oct 12, 2021
Closed
@daxfohl
Copy link
Contributor

daxfohl commented Mar 16, 2022

@tanujkhattar this was done in #5075, so can be closed.

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

@maffoo maffoo maffoo left review comments

@tanujkhattar tanujkhattar tanujkhattar requested changes

@dstrain115 dstrain115 dstrain115 approved these changes

@cduck cduck Awaiting requested review from cduck

@vtomole vtomole Awaiting requested review from vtomole

@wcourtney wcourtney Awaiting requested review from wcourtney

Assignees

@tanujkhattar tanujkhattar

Labels
cla: yes Makes googlebot stop complaining. size: S 10< lines changed <50
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Modify type annotation in FSimGate constructor from float to TParamVal
6 participants
@stubbi @tanujkhattar @daxfohl @maffoo @dstrain115 @CirqBot