Skip to content

Allow symbol for coupling_mhz in CouplerPulse gate#5908

Merged
maffoo merged 4 commits intomasterfrom
u/maffoo/coupler-pulse
Oct 29, 2022
Merged

Allow symbol for coupling_mhz in CouplerPulse gate#5908
maffoo merged 4 commits intomasterfrom
u/maffoo/coupler-pulse

Conversation

@maffoo
Copy link
Copy Markdown
Contributor

@maffoo maffoo commented Oct 3, 2022

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

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