-
Notifications
You must be signed in to change notification settings - Fork 984
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
Fix __radd__ losing device #2451
Conversation
result = Circuit(other) | ||
return result.__iadd__(self) | ||
result = self.copy() | ||
result._moments[:0] = Circuit(other)._moments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't appear to validate the new moment against the device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is validating the entire new circuit needed? I would just validate the new moment:
result._moments[:0] = Circuit(other, device=self.device)._moments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are constraints on operations that can be applied in sequence on the same qubits for certain devices, so validating the whole circuit is necessary to catch that issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but Circuit.insert only validates the moment. This is a separate issue from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validation added and tested.
result = Circuit(other) | ||
return result.__iadd__(self) | ||
result = self.copy() | ||
result._moments[:0] = Circuit(other)._moments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is validating the entire new circuit needed? I would just validate the new moment:
result._moments[:0] = Circuit(other, device=self.device)._moments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, LGTM.
result = Circuit(other) | ||
return result.__iadd__(self) | ||
result = self.copy() | ||
result._moments[:0] = Circuit(other)._moments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but Circuit.insert only validates the moment. This is a separate issue from this PR.
Automerge cancelled: A status check is failing. |
Automerge cancelled: A status check is failing. |
No description provided.