-
Notifications
You must be signed in to change notification settings - Fork 1
Test basic methods of all squin statements #436
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
base: main
Are you sure you want to change the base?
Conversation
Yet another bug found in #436
Another thing that came out of #436. --------- Co-authored-by: Phillip Weinberg <[email protected]>
Another thing that came out of #436. --------- Co-authored-by: Phillip Weinberg <[email protected]>
@johnzl-777 not sure what the problem with the |
Wait @david-pl I assume (especially from the comment) you still need the list of supported statements that can be translated to stim, right? Otherwise this definitely won't go through as-is 😅 |
@johnzl-777 yes, definitely. But I'm not 100% sure how the error in the test is related. |
Alright, looks like the squin2stim test is the last one we need to fix, all other tests pass already. |
One of the things that dropped out of #436
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
@david-pl I'm still not quite sure why That being said just a couple observations on my end:
|
@johnzl-777 thanks for updating the test! I'm responding point-by-point here:
|
Draft for #435
Looks like this was long overdue. It fails currently because:
Depolarize2
(this also means pyqrack simulation fails)Rot
(wow, that's on me) andResetToOne
.Squin2StimPass
which causes an error.The only thing that seems to work right now is the
AddressAnalysis
(kudos @johnzl-777).@kaihsin @johnzl-777 the test for the
Squin2StimPass
still needs some work to properly test stuff, but I didn't want to mix commits and start fixing bugs right now, so we need to add that later.FYI, I added a little rewrite pass just for testing that removes intentionally unsupported statements in case that's relevant for the stim tests. We should probably document these statements somewhere, but right now it's basically just atom loss that is unsupported in cirq.
Open questions:
Edit: After @johnzl-777 pointed to them in #435, I also added tests for the
MeasurementIDAnalysis
(which works) and theNSitesAnalysis
(currently chokes onPauliString
).