-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Remove uses of .device
in cirq-pasqal in preparation for larger circuit.device deprecation
#4757
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
Changes from all commits
1752040
f63befc
e518ca1
d85244b
3d24070
f82487b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,15 +21,20 @@ | |
|
||
|
||
class PasqalSampler(cirq.work.Sampler): | ||
def __init__(self, remote_host: str, access_token: str = '') -> None: | ||
def __init__( | ||
self, remote_host: str, access_token: str = '', device: cirq_pasqal.PasqalDevice = None | ||
) -> None: | ||
"""Inits PasqalSampler. | ||
|
||
Args: | ||
remote_host: Address of the remote device. | ||
access_token: Access token for the remote api. | ||
device: Optional cirq_pasqal.PasqalDevice to use with | ||
the sampler. | ||
""" | ||
self.remote_host = remote_host | ||
self._authorization_header = {"Authorization": access_token} | ||
self._device = device | ||
|
||
def _serialize_circuit( | ||
self, | ||
|
@@ -100,6 +105,20 @@ def _send_serialized_circuit( | |
|
||
return result | ||
|
||
@cirq._compat.deprecated_parameter( | ||
deadline='v0.15', | ||
fix='The program.device component is going away.' | ||
'Attaching a device to PasqalSampler is now done in __init__.', | ||
parameter_desc='program', | ||
match=lambda args, kwargs: ( | ||
len(args) >= 2 | ||
and isinstance(args[1], cirq.AbstractCircuit) | ||
and args[1]._device != cirq.UNCONSTRAINED_DEVICE | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can _device be None? I see it set to None above but not to cirq.UNCONSTRAINED_DEVICE. Same with checks below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The The |
||
) | ||
or 'program' in kwargs | ||
and isinstance(kwargs['program'], cirq.AbstractCircuit) | ||
and kwargs['program']._device != cirq.UNCONSTRAINED_DEVICE, | ||
) | ||
def run_sweep( | ||
self, program: cirq.AbstractCircuit, params: cirq.study.Sweepable, repetitions: int = 1 | ||
) -> List[cirq.study.Result]: | ||
|
@@ -114,8 +133,11 @@ def run_sweep( | |
Result list for this run; one for each possible parameter | ||
resolver. | ||
""" | ||
assert isinstance(program.device, cirq_pasqal.PasqalDevice) | ||
program.device.validate_circuit(program) | ||
device = program._device if program._device != cirq.UNCONSTRAINED_DEVICE else self._device | ||
assert isinstance( | ||
device, cirq_pasqal.PasqalDevice | ||
), "Device must inherit from cirq.PasqalDevice." | ||
device.validate_circuit(program) | ||
trial_results = [] | ||
|
||
for param_resolver in cirq.study.to_resolvers(params): | ||
|
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.
We do require a
PasqalDevice
to be specified. If that's not going to be done inCircuit
anymore,device
shouldn't be optional and it must be aPasqalDevice
instance.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.
I can add the assertion checks at init to make sure it's a pasqal device.
This would make this a breaking change. If this PR were to be merged, both the old and new functionality would still work. Until the deprecation warning is up a user could still attach a device to a circuit or construct a sampler with a device (sampler device taking precedence here). Would you prefer we do a breaking style change and make this argument required ?
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.
I see what you mean. While the option to attach a device to a circuit is still there, this doesn't need to be required. What if you keep it
Optional
for now, but demand that there is aPasqalDevice
associated? If aPasqalDevice
is attached to the circuit, this doesn't have to be specified. Otherwise, it is required. Later on, when the option to have a device attached to a circuit is gone, this is turned into a required argument.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.
Did you consider this option?
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.
I believe so. To me this sounds like what is currently implemented in the code. Re-reading it now I'm having trouble figuring out what we're missing, would you mind adding a little snippet here in python or pseudocode of what changes we should consider to help me understand ?
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.
Sure, see my comment on lines 136 to 142.