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

Create a transformer cancels the effect of Z-phases #6837

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

NoureldinYosri
Copy link
Collaborator

No description provided.

@CirqBot CirqBot added the size: M 50< lines changed <250 label Dec 11, 2024
@NoureldinYosri NoureldinYosri marked this pull request as ready for review December 11, 2024 22:26
@NoureldinYosri NoureldinYosri requested review from mrwojtek, vtomole and a team as code owners December 11, 2024 22:26
*,
context: Optional[transformer_api.TransformerContext] = None,
) -> 'cirq.Circuit':
"""Replace every occurance of a calibrated gate with a proper replacement.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a more descriptive docstring?

Comment on lines 353 to 355
theta=self.target_as_fsim.theta - delta_theta,
gamma=self.target_as_fsim.gamma - delta_gamma,
phi=self.target_as_fsim.phi - delta_phi,
Copy link
Collaborator

Choose a reason for hiding this comment

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

theta and phi are not z-phases. Can we directly insert the z-rotations that take care of cancelling the phases (just the other three angles)?

Copy link
Collaborator Author

@NoureldinYosri NoureldinYosri Dec 13, 2024

Choose a reason for hiding this comment

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

theta and phi are not z-phases.

yes, but if they were calibrated then it makes sense to change them as well. if they were not calibrated then the delta would be = 0 and nothing happens.

Can we directly insert the z-rotations that take care of cancelling the phases (just the other three angles)?

we can, I'm okay either way

Copy link
Collaborator

@eliottrosenberg eliottrosenberg Dec 13, 2024

Choose a reason for hiding this comment

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

In order for this to be ready to run on hardware, it would be great if there were at least an option to only cancel the z-phases (not theta and phi) and for this to be implemented as z rotations rather than fSim gates. Thanks, Nour.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ptal

@NoureldinYosri NoureldinYosri changed the title Create a transformer to replace gates with their calibrated versions Create a transformer cancels the effect of Z-phases Dec 14, 2024
Copy link
Collaborator

@eliottrosenberg eliottrosenberg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks, Nour!

Tested in this colab.

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.86%. Comparing base (2b19bd3) to head (8a400d6).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6837   +/-   ##
=======================================
  Coverage   97.86%   97.86%           
=======================================
  Files        1084     1084           
  Lines       93954    94034   +80     
=======================================
+ Hits        91944    92024   +80     
  Misses       2010     2010           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants