-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add classical variable support to qasm3_qir_converter
#85
Conversation
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.
Thanks for these updates. Gave a couple suggestions for error handling using Exceptions instead of assert. Also consider using collections.deque
for the scope dict list. And finally, if you could just add test cases for any new code that would be great.
Thanks for the suggestions! Was still a WIP so that's why haven't added tests yet. Will update with the changes and tests |
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.
Updates look good! The _get_scope
method has a bug that will need to be fixed, and offered a couple other optional suggestions for some of the other scope methods.
If you could provide some clarity on the left / right variable constants that you're multiplying that would be helpful.
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.
Looks good to me!
Yea thanks, had missed this debug statement. |
95e9b3f
to
deba11e
Compare
deba11e
to
5219c08
Compare
Hey @ryanhill1 , after reviewing you can go ahead and merge. Will take up the last three tasks in another PR as this is getting too big for one feature! I have created a new issue #90 with the leftover stuff |
Fixes #76
Changes