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

Create __add__ method for Moment #2665

Merged
merged 8 commits into from
Jan 17, 2020
Merged

Create __add__ method for Moment #2665

merged 8 commits into from
Jan 17, 2020

Conversation

fedimser
Copy link
Collaborator

It will be an alias for with_operation method.

Fix for #2641 .

@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Dec 25, 2019
@vtomole vtomole requested a review from Strilanc December 26, 2019 17:01
vtomole
vtomole requested changes Dec 26, 2019
View reviewed changes
Copy link
Collaborator

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

cirq/ops/moment.py Outdated Show resolved Hide resolved
cirq/ops/moment_test.py Show resolved Hide resolved
vtomole
vtomole previously requested changes Dec 27, 2019
View reviewed changes
Copy link
Collaborator

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

Sorry if i wasn't clear. Just remove the else. The Not implemented stays. Basically the else statement is unnecessary because that if will return and if it doesn't, the not implemented will return.

@fedimser
Copy link
Collaborator Author

I see. I put NotImplemented back, without else.
Actually, this is important, as failed test have shown (because Moment can also be added to Circuit, and it can be left operand).

viathor
viathor approved these changes Jan 15, 2020
View reviewed changes
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

It seems all the comments from @vtomole have been addressed.

@vtomole vtomole self-requested a review January 15, 2020 21:06
@vtomole vtomole dismissed their stale review January 15, 2020 21:07

Adressed

@vtomole
Copy link
Collaborator

vtomole commented Jan 15, 2020

I can't update this branch to master. @fedimser Can you?

@fedimser
Copy link
Collaborator Author

Hi.
Can please someone with write aceess merge this pull request?

@vtomole vtomole added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jan 16, 2020
@vtomole
Copy link
Collaborator

vtomole commented Jan 16, 2020

@Strilanc Bot is down.

@vtomole vtomole merged commit 266491a into quantumlib:master Jan 17, 2020
@vtomole vtomole mentioned this pull request Jan 17, 2020
Closed
@fedimser fedimser deleted the moment_add branch January 19, 2020 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viathor viathor viathor approved these changes

@vtomole vtomole vtomole approved these changes

@Strilanc Strilanc Awaiting requested review from Strilanc

Assignees
No one assigned
Labels
automerge Tells CirqBot to sync and merge this PR. (If it's running.) cla: yes Makes googlebot stop complaining.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fedimser @vtomole @viathor @googlebot