-
Notifications
You must be signed in to change notification settings - Fork 615
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 parameter counting in DefaultQubitLegacy
adjoint method
#4820
Conversation
@vincentmr This is the PR (sorry, took a moment to figure this out after all) |
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.
Even though its currently not causing issues, mind fixing this as well in the new system:
if op.grad_method is not None: |
if op.grad_method is not None: |
if op.grad_method is not None: |
@albi3ro Done :) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4820 +/- ##
==========================================
- Coverage 99.64% 99.64% -0.01%
==========================================
Files 381 381
Lines 34377 34117 -260
==========================================
- Hits 34256 33995 -261
- Misses 121 122 +1
☔ View full report in Codecov by Sentry. |
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.
great find, and simple fix!
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 getting this together so quickly.
Context:
The adjoint method of
DefaultQubitLegacy
uses two incompatible ways to quantify tape parameters: It sets up the number of tape parameters correctly usingtape.get_parameters(trainable_only=False)
but checks whether an operation comes with a parameter viagrad_method is not None
. This lead to a bug where, for example,QubitUnitary
disrupts the parameter counting.Description of the Change:
As all operations with more than one parameter are being decomposed/raised an error upon anyways, this PR changes the way to detect parametrized operations to
op.num_params==1
.Benefits:
Bug fix: Correct adjoint method derivatives even when using
QubitUnitary
and other operations that havegrad_method=None
but have parameters.Possible Drawbacks:
N/A
Related GitHub Issues:
N/A