-
Notifications
You must be signed in to change notification settings - Fork 50
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
fix #191: too restrictive typeassert for MixedDestabilizer #366
Conversation
The PR is ready for review. Thank you! P.S. My bad for the rebase. Initially, I had used |
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.
Could you elaborate why this is necessary? Maybe add tests that would have failed without this change?
The proper fix for this probably requires better parameterization of the input argument. The issue is that the following
Rather what is preserved is the
Maybe we should define
|
Codecov ReportAll modified and coverable lines are covered by tests โ
Additional details and impacted files@@ Coverage Diff @@
## master #366 +/- ##
==========================================
+ Coverage 82.42% 83.97% +1.55%
==========================================
Files 70 70
Lines 4580 4918 +338
==========================================
+ Hits 3775 4130 +355
+ Misses 805 788 -17 โ View full report in Codecov by Sentry. |
I attempted the suggested approach by defining the following method:
Afterward, I initialized a variable
However, when executing
I have followed your suggestions from #366 (review) by including a comment to elaborate the changes. I hope this is reasonable. |
Thank you! I have added tests that fail on the master branch, which reproduce the error without the need for fancy expressions like
Consequently, the suggestions from #366 (review) have been fully incorporated. Thus, this PR resolves #191
|
โฆimproved since this was necessary; also simplified the tests
Yup, the JET and compiler have improved as previously removing Thank you for the simplification. |
Great! Thanks for the fix! |
This now works and all the test passes as well:
The JET errors does not increase as well. Hopefully, this is plausible.