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.

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

Merged
merged 4 commits into from
Oct 29, 2022

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented Oct 3, 2022

This is something we use on our internal version of this gate.

@maffoo maffoo requested review from wcourtney, a team, vtomole, cduck and verult as code owners October 3, 2022 22:14
@CirqBot CirqBot added the Size: XS <10 lines changed label Oct 3, 2022
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.

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)‾‾\───

@CirqBot CirqBot added the size: M 50< lines changed <250 label Oct 28, 2022
@maffoo
Copy link
Contributor Author

maffoo commented Oct 28, 2022

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 (parameter_names and is_parameterized). PTAL

@maffoo maffoo requested a review from vtomole October 28, 2022 20:29
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.

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})'
        )

Comment on lines 97 to 100
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)
Copy link
Collaborator

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

Suggested change
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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@maffoo maffoo requested a review from vtomole October 28, 2022 23:32
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.

LGTM

@maffoo maffoo merged commit 19e7a42 into master Oct 29, 2022
@maffoo maffoo deleted the u/maffoo/coupler-pulse branch October 29, 2022 13:41
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250 Size: XS <10 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants