-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor cirq - squin emit and lowering #520
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
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
@Roger-luo @david-pl This PR has to go before #518 because qubit.new touches a alot of the code base including cirq lowering. |
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, @Roger-luo, I know we had discussed doc-strings on the new API, but here all the visitor methods and interpreter methods are not Private. Should we mark the classes as private in this case?
@weinbe58 @Roger-luo I went ahead and marked the method tables as private, since there's really no point in having those public. I'm not so sure about the target classes themselves though, so let me know how to proceed here. I'll still go ahead and merge so as to not block other progress in the refactor. We can still change this during the refactor to make it private or add docstrings. |
Closes #497
I had to cut down support for some cirq features in lowering, but I don't think they were used heavily. For example, we no longer have
CZPowGate
with arbitrary exponents or arbitrary controlled operations.Also, one more significant breaking change is that
func.Invoke
statements are no longer emitted asCircuitOperation
, but are added to the circuit flat-out. I think this matches user expectations and otherwise would have been really odd given our new approach with stdlib functions.Finally, noise still needs to be done, but that is pending #495, which should be merged first, I think.
Edit: Noise is also done now, but I also had to cut support for
AmplitudeDampingChannel
, since we can't really represent this with the current statements in the noise dialect (I think). However, I haven't seen this used anywhere so far, so I'd suggest we wait for someone to actually require that feature.