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

[XEB] Allow default angles to be inferred from a gate #4092

Merged
merged 2 commits into from
May 10, 2021

Conversation

mpharrigan
Copy link
Collaborator

Instead of defaulting angles to 0.0 -- which is a very sharp edge that can cause optimization to fail -- default to None and either require the user fully specify the angle settings or provide a gate to get sensible defaults from. In xeb_wrapper this gate is already around as the calibration request gate.

@mpharrigan mpharrigan requested a review from mrwojtek May 7, 2021 00:32
@mpharrigan mpharrigan requested review from cduck, vtomole and a team as code owners May 7, 2021 00:32
@mpharrigan mpharrigan requested a review from dstrain115 May 7, 2021 00:32
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label May 7, 2021
'phi_default': 0.0,
}
if gate == SQRT_ISWAP:
defaults['theta_default'] = -np.pi / 4
Copy link
Collaborator

@mrwojtek mrwojtek May 7, 2021

Choose a reason for hiding this comment

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

I think we should set also phi_default to np.pi / 24 here and below. This might improve the optimization speed? I'll try this setting as soon I have a chance. We can change this in a follow-up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this would be a dangerous semantic to introduce in cirq-core, which is where this function currently lives. Please be aware that if you set characterize_phi=False, the fixed value will be set to phi_default. I don't know if this would help or hurt when you're only optimizing phases.

Options:

  1. [my preference] have a wrapper like SqrtISwapXEBOptions (GoogleSqrtISwapXEBOptions) that sets {angle}_default values and tell users to use it instead of the automatically-determine-from-gate when you're using SqrtIswap on a google processor because it can help with phi characterization
  2. Inject a different phased_fsim_angles_from_gate if you're running via cirq_google.workflow
  3. [radical] Introduce a new sqrt-iswap-like gate defined to ideally have phi of np.pi/24 and have people write their algorithms with this gate. Then the angle determination would work automatically. You could still treat it as an iswap gate but you'd have to explicitly neglect the small conditional phase

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like option 2 but maybe let's play with this future a little bit if that would make any difference at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I included the gate-angle-inference function as an argument to the method to support option 2

@mpharrigan
Copy link
Collaborator Author

I manually checked notebooks as well since I was suspicious they didn't require any changes, but they all use SqrtISwapOptions which is fully backwards compatible. Note: this is a breaking change if you were previously only supplying some of the angles when manually constructing PhasedFSimCharacterizationOptions.

@mpharrigan mpharrigan added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 10, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 10, 2021
@CirqBot CirqBot merged commit 2f8326c into quantumlib:master May 10, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels May 10, 2021
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Instead of defaulting angles to 0.0 -- which is a very sharp edge that can cause optimization to fail -- default to `None` and either require the user fully specify the angle settings or provide a gate to get sensible defaults from. In `xeb_wrapper` this gate is already around as the calibration request gate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/xeb cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants