-
Notifications
You must be signed in to change notification settings - Fork 591
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
Support wire labels in qinfo
transforms
#4331
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #4331 +/- ##
=======================================
Coverage 99.79% 99.79%
=======================================
Files 351 351
Lines 32087 32096 +9
=======================================
+ Hits 32020 32029 +9
Misses 67 67
|
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 the super quick fix @eddddddy 💯
Thorough test additions 😎
I only have two very minor suggestions.
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.
I think the PR looks good. But two things concern me w.r.t the new device API:
- The new device doesn't have wires, so
qnode.device.wires
can't be used for wire mapping. - The new device also does not have a
C_DTYPE
orR_DTYPE
attribute, which I'm seeing being used in the qinfo transforms.
Tagging @albi3ro for thoughts.
Once these are addressed, I'm happy to approve. Everything else looked good :)
I think the qinfo module (and various other ones) are still incompatible with the new device API, and it'll be quite a bit of work to get things caught up unfortunately. I'd prefer that this PR isn't blocked by that, since it'll need to be handled at a larger scale anyway |
I agree with @timmysilv. There are still some things to work out for compatibility with the new device API and I'd rather we do those in a separate PR that includes everything else. |
[sc-41189] |
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.
🚀
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.
As others have pointed out, these transforms will cause issues with the new device API. I've created a new shortcut ticket to resolve these issues, the most tricky of which will be the wire order for a state vector with non-standard labels.
But that shouldn't block this bugfix right now, as long as the other open questions are resolved before the next release.
Thanks for fixing this up :)
* Support wire labels in qinfo transforms * changelog * pylint and update test * add bugfix entry
* Adding changes for shift rules * Testing changes * Fixed op indices * Fixed indexing * Updated `bind_new_parameters` * Updated `tape.get_operation` * Updated tests * Test updates; multishifting works * Fixing interface * Updated shifting; added dispatch for templates * Updated shifting function * Fixed index error * Removed commented code * [skip ci] Reverted changes to `bind_new_parameters * Update to remove copying * Update changelog * Apply suggestions from code review Co-authored-by: Matthew Silverman <matthews@xanadu.ai> * Removed unused import * Roll back suggested change * Remove state vector support from `math/quantum.py` (#4322) * Remove statevector support for qinfo functions * remove unused funcs * Fix some tests * pylint * more pylint * Remove unnecessary log * Changelog and deprecations entry * trigger ci * Enable CI and pre-commit hook to lint tests (#4335) * rename all legacy test files to match pylint pattern * pylint all tests in CI and pre-commit * lint legacy/qnn/conftest * remove the custom pylint test handling * run black before pylint * changelog --------- Co-authored-by: David Wierichs <david.wierichs@xanadu.ai> * Update broadcasting transforms to use `bind_new_parameters` (#4288) * use bind_new_parameters * pylint * remove batching in measurements and add tests * More coverage issues * Add uncopied tests * Helper function * pylint * Make tape._ops public * use private methods * pylint * Add more docs to split_operations * pylint * Deprecate X and P (#4330) * update in docs * update changelog * Test warning is raised * update default gaussian device * update tests * update more tests * fix legacy tests * Specify v0.33 removal in docstring * Render X and P in docs * move deprecation warning in docstring * fix sphinx linking * add warning box * Support wire labels in `qinfo` transforms (#4331) * Support wire labels in qinfo transforms * changelog * pylint and update test * add bugfix entry * Deprecations for 0.32 from me! (#4316) * deprecate the old return system * deprecate the mode kwarg for QNode * changelog * PR feedback * update notice to avoid wrongly suggesting action needed * update docstrings, docs and warnings * add link to qnode returns doc * change the mode warning depending on return system active * also add disclaimer to docstring --------- Co-authored-by: Matthew Silverman <matthews@xanadu.ai> Co-authored-by: Edward Jiang <34989448+eddddddy@users.noreply.github.com> Co-authored-by: David Wierichs <david.wierichs@xanadu.ai> Co-authored-by: lillian542 <38584660+lillian542@users.noreply.github.com>
Context:
Taken from #4318:
Description of the Change:
Support custom wire labels in all
qinfo
transforms.Related GitHub Issues:
Fixes #4318