fix for issue #4087 #4103

NoureldinYosri · 2021-05-12T19:13:40Z

Problem: the PauliSum add function checks that the type of the other operand is one of numbers.Complex, PauliString, PauliSum and raises NotImplemented otherwise.
Fix: if the other operand is not one of the types try to cast it to PauliString and raise error only if that fails

Fixes #4087.

balopat · 2021-05-14T23:54:05Z

Problem: the PauliSum add function checks that the type of the other operand is one of numbers.Complex, PauliString, PauliSum and raises NotImplemented otherwise.
Fix: if the other operand is not one of the types try to cast it to PauliString and raise error only if that fails

Hi, thank you for taking this up! Can you please add a test that demonstrates and protects this functionality?

NoureldinYosri · 2021-05-15T16:04:23Z

Hi, I added a test, please let me know of any suggestions :)

NoureldinYosri · 2021-05-19T13:13:34Z

@balopat any updates ?

balopat · 2021-05-19T17:26:10Z

This looks good, but it seems that there are failing tests...

NoureldinYosri · 2021-05-20T19:00:48Z

@balopat
looks like the only failing test is this one https://github.com/quantumlib/Cirq/blob/master/cirq-core/cirq/circuits/circuit_test.py#L299 where in this line https://github.com/quantumlib/Cirq/blob/master/cirq-core/cirq/circuits/circuit_test.py#L310 it expects the result of addition to be

cirq.Circuit([
     cirq.Moment(
         cirq.X(cirq.NamedQubit('a')),
     ),
 ],
)

but after fixing the addition operator it gives

cirq.PauliSum(cirq.LinearDict({frozenset({(cirq.NamedQubit('a'), cirq.X)}): (1+0j), frozenset(): (1+0j)})

so before I dive deeper in debugging Is this the intended behaviour ?

review-notebook-app · 2021-05-25T21:21:02Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

google-cla · 2021-05-25T21:21:21Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

NoureldinYosri · 2021-05-25T21:23:47Z

@balopat I restricted the casting to only happen for gate operations, this should fix the issue
PS: I probably did something wrong with git that made it attach unrelated comments to this thread.

google-cla · 2021-05-25T21:24:21Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2021-05-25T21:31:57Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

… a PauliSum is a valid operation

google-cla · 2021-05-26T18:33:56Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2021-05-26T18:35:05Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

maffoo · 2021-05-26T18:57:43Z

Ref: #4087

cirq-core/cirq/ops/linear_combinations_test.py

google-cla · 2021-05-26T19:12:28Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2021-05-26T19:12:36Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2021-05-27T08:13:40Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

NoureldinYosri · 2021-06-11T11:50:09Z

@vtomole @balopat a gentle reminder about this PR.

balopat · 2021-06-11T22:05:35Z

cirq-core/cirq/ops/linear_combinations.py

@@ -570,6 +570,8 @@ def __iadd__(self, other):
        return self

    def __add__(self, other):
+        if isinstance(other, (gate_operation.GateOperation)):


As discussed on the Cirq Cynque, we'll need to make this more specific.
Namely, the instance check should be about checking whether this is an instance of identity.IdentityGate. After this I think we're done with this PR :)

but that won't fix the issue in #4087 , the identity there is constructed as cirq.I(cirq.LineQubit(0)) which has type cirq.ops.gate_operation.GateOperation not cirq.ops.identity.IdentityGate, so checking the instance type will fail.

Oh, yes, I meant instance GateOperation, other.gate is instance of IdentityGate.

updated (y).

google-cla · 2021-06-28T14:59:17Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2021-06-28T15:04:14Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2021-06-28T15:07:14Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2021-06-28T15:33:49Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

addressed

google-cla · 2021-07-02T13:16:37Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Problem: the PauliSum add function checks that the type of the other operand is one of numbers.Complex, PauliString, PauliSum and raises NotImplemented otherwise. Fix: if the other operand is not one of the types try to cast it to PauliString and raise error only if that fails Fixes quantumlib#4087.

NoureldinYosri requested review from cduck, vtomole and a team as code owners May 12, 2021 19:13

NoureldinYosri requested a review from dstrain115 May 12, 2021 19:13

google-cla bot added the cla: yes Makes googlebot stop complaining. label May 12, 2021

balopat approved these changes May 19, 2021

View reviewed changes

NoureldinYosri requested a review from wcourtney as a code owner May 25, 2021 21:20

google-cla bot added cla: no and removed cla: yes Makes googlebot stop complaining. labels May 25, 2021

added fix for quantumlib#4087 and a test to ensure adding identity to…

4889170

… a PauliSum is a valid operation

NoureldinYosri force-pushed the fixPauliSumAdd branch from a8136aa to 4889170 Compare May 26, 2021 18:33

google-cla bot added cla: yes Makes googlebot stop complaining. and removed cla: no labels May 26, 2021

vtomole previously requested changes May 26, 2021

View reviewed changes

cirq-core/cirq/ops/linear_combinations_test.py Outdated Show resolved Hide resolved

removed try/except clause

9c55a61

NoureldinYosri force-pushed the fixPauliSumAdd branch from 2852567 to 9c55a61 Compare May 26, 2021 19:12

vtomole self-requested a review May 26, 2021 19:52

balopat reviewed Jun 11, 2021

View reviewed changes

NoureldinYosri force-pushed the fixPauliSumAdd branch from 0645026 to 312d175 Compare June 28, 2021 14:59

NoureldinYosri force-pushed the fixPauliSumAdd branch from 312d175 to cea6c7a Compare June 28, 2021 15:04

NoureldinYosri force-pushed the fixPauliSumAdd branch from cea6c7a to d3a09b6 Compare June 28, 2021 15:07

restrict identity check to other.gate

21c813d

NoureldinYosri force-pushed the fixPauliSumAdd branch from d3a09b6 to 21c813d Compare June 28, 2021 15:33

balopat self-assigned this Jul 1, 2021

balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 2, 2021

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 2, 2021

Merge branch 'master' into fixPauliSumAdd

6a15d51

CirqBot merged commit 6b6fc98 into quantumlib:master Jul 2, 2021

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 Jul 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for issue #4087 #4103

fix for issue #4087 #4103

NoureldinYosri commented May 12, 2021 •

edited by balopat

Loading

balopat commented May 14, 2021

NoureldinYosri commented May 15, 2021

NoureldinYosri commented May 19, 2021

balopat commented May 19, 2021

NoureldinYosri commented May 20, 2021

review-notebook-app bot commented May 25, 2021

google-cla bot commented May 25, 2021

NoureldinYosri commented May 25, 2021 •

edited

Loading

google-cla bot commented May 25, 2021

googlebot commented May 25, 2021

google-cla bot commented May 26, 2021

google-cla bot commented May 26, 2021

maffoo commented May 26, 2021

google-cla bot commented May 26, 2021

google-cla bot commented May 26, 2021

google-cla bot commented May 27, 2021

NoureldinYosri commented Jun 11, 2021

balopat Jun 11, 2021

NoureldinYosri Jun 12, 2021

balopat Jun 28, 2021

NoureldinYosri Jun 28, 2021

google-cla bot commented Jun 28, 2021

google-cla bot commented Jun 28, 2021

google-cla bot commented Jun 28, 2021

google-cla bot commented Jun 28, 2021

google-cla bot commented Jul 2, 2021

fix for issue #4087 #4103

fix for issue #4087 #4103

Conversation

NoureldinYosri commented May 12, 2021 • edited by balopat Loading

balopat commented May 14, 2021

NoureldinYosri commented May 15, 2021

NoureldinYosri commented May 19, 2021

balopat commented May 19, 2021

NoureldinYosri commented May 20, 2021

review-notebook-app bot commented May 25, 2021

google-cla bot commented May 25, 2021

NoureldinYosri commented May 25, 2021 • edited Loading

google-cla bot commented May 25, 2021

googlebot commented May 25, 2021

google-cla bot commented May 26, 2021

google-cla bot commented May 26, 2021

maffoo commented May 26, 2021

google-cla bot commented May 26, 2021

google-cla bot commented May 26, 2021

google-cla bot commented May 27, 2021

NoureldinYosri commented Jun 11, 2021

balopat Jun 11, 2021

Choose a reason for hiding this comment

NoureldinYosri Jun 12, 2021

Choose a reason for hiding this comment

balopat Jun 28, 2021

Choose a reason for hiding this comment

NoureldinYosri Jun 28, 2021

Choose a reason for hiding this comment

google-cla bot commented Jun 28, 2021

google-cla bot commented Jun 28, 2021

google-cla bot commented Jun 28, 2021

google-cla bot commented Jun 28, 2021

google-cla bot commented Jul 2, 2021

NoureldinYosri commented May 12, 2021 •

edited by balopat

Loading

NoureldinYosri commented May 25, 2021 •

edited

Loading