-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Deprecate ciruit.device
everywhere.
#4845
Deprecate ciruit.device
everywhere.
#4845
Conversation
if self._device == cirq.UNCONSTRAINED_DEVICE: | ||
return FrozenCircuit(self, strategy=InsertStrategy.EARLIEST) | ||
return FrozenCircuit(self, strategy=InsertStrategy.EARLIEST, device=self._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.
The reason I opted for this patter instead of something like:
ret_device = FrozenCircuit(self, strategy=....)
ret_device._device = self._device
return ret_device
Is because the constructor with the device argument makes non trivial behavior changes to the constructor and so that's why I opted to use this pattern in places with deprecations. Removing them once the cycle is over should be relatively easy to do since all we need to do is keep the snippet inside the if statement and delete the else block.
cirq-core/cirq/circuits/circuit.py
Outdated
@@ -70,6 +73,17 @@ | |||
_DEVICE_DEP_MESSAGE = 'Attaching devices to circuits will no longer be supported.' | |||
|
|||
|
|||
@contextlib.contextmanager | |||
def _block_overlapping_dep(): |
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.
What's up with this? Perhaps a line comment explaining what this is.
Also I would change dep to deprecation, since abbreviations should be avoided, especially ambiguous ones (could be dependencies)
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 is to catch overlapping deprecation warnings. If a user calls with_device they should only see the warning for with_device
and not the internal message of with_device using the constructor with the device argument as well.
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.
One concern re: JSON omission of device
, otherwise LGTM
cirq-core/cirq/circuits/circuit.py
Outdated
@@ -1323,11 +1343,11 @@ def save_qasm( | |||
self._to_qasm_output(header, precision, qubit_order).save(file_path) | |||
|
|||
def _json_dict_(self): | |||
return protocols.obj_to_dict_helper(self, ['moments', 'device']) | |||
return protocols.obj_to_dict_helper(self, ['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 confuses me a bit: we still support device
everywhere, except for JSON? It seems like this and _from_json_dict_
should continue supporting device
until it's removed everywhere else as well.
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.
Good point, got a little ahead of myself there. Went ahead and put device back in json. We can cut it and add the {repr,json}_inward once things are officially cut when the deprecation cycle is over.
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.
LGTM
Last item in step 1 of quantumlib#4744 .
Last item in step 1 of quantumlib#4744 .
Last item in step 1 of quantumlib#4744 .
Last item in step 1 of #4744 .