-
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
Allow symbol for coupling_mhz in CouplerPulse gate #5908
Conversation
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.
If you want to allow the passing of symbolic expressions, you won't be able to evaluate the repr
gate = coupler_pulse.CouplerPulse(
hold_time=cirq.Duration(nanos=10), coupling_mhz=sympy.cos(sympy.Symbol('x')) + 1 , rise_time=cirq.Duration(nanos=18)
)
cirq.testing.assert_implements_consistent_protocols(
gate,
setup_code='import cirq\nimport numpy as np\nimport sympy\nimport cirq_google',
qubit_count=2,
ignore_decompose_to_default_gateset=True,
)
Fails with a NameError("name 'cos' is not defined")
.
The resolver doesn't seem to work with this change
circuit = cirq.Circuit(gate.on(cirq.LineQubit(0), cirq.LineQubit(1)))
print(cirq.resolve_parameters(circuit, {"x": 0.1}))
# prints
# 0: ───/‾‾(10 ns@cos(x) + 1MHz)‾‾\───
# │
# 1: ───/‾‾(10 ns@cos(x) + 1MHz)‾‾\───
Thanks for taking a look, @vtomole. I don't think repr evaluate is particularly important and I presume the same thing would fail for other parameterized gates as well. I did add support for parameter resolution and related protocols ( |
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 don't think repr evaluate is particularly important
Not important, but should be as simple as modifying CouplerPulse
's repr
to use proper_repr
from cirq._compat import proper_repr
def __repr__(self) -> str:
return (
'cirq_google.experimental.ops.coupler_pulse.'
+ f'CouplerPulse(hold_time={self.hold_time!r}, '
+ f'coupling_mhz={proper_repr(self.coupling_mhz)}, '
+ f'rise_time={self.rise_time!r}, '
+ f'padding_time={self.padding_time!r})'
)
cirq.is_parameterized(self.hold_time) | ||
or cirq.is_parameterized(self.rise_time) | ||
or cirq.is_parameterized(self.padding_time) | ||
or cirq.is_parameterized(self.coupling_mhz) |
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.
Nit: For consistency with the __init__
params
cirq.is_parameterized(self.hold_time) | |
or cirq.is_parameterized(self.rise_time) | |
or cirq.is_parameterized(self.padding_time) | |
or cirq.is_parameterized(self.coupling_mhz) | |
cirq.is_parameterized(self.hold_time) | |
or cirq.is_parameterized(self.coupling_mhz) | |
or cirq.is_parameterized(self.rise_time) | |
or cirq.is_parameterized(self.padding_time) |
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.
Done.
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
This is something we use on our internal version of this gate.