-
Notifications
You must be signed in to change notification settings - Fork 603
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 jacobian shape in VJP for measurements with shape and batched inputs #5986
Fix jacobian shape in VJP for measurements with shape and batched inputs #5986
Conversation
This commit takes the proper shape of the jacobian when having - batching in non-trainable data - Measurement with a shape (ie probs) - More than one trainable parameter in the circuit
ccb773a
to
a84ad45
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5986 +/- ##
==========================================
- Coverage 99.66% 99.65% -0.01%
==========================================
Files 430 430
Lines 41544 41281 -263
==========================================
- Hits 41404 41140 -264
- Misses 140 141 +1 ☔ View full report in Codecov by Sentry. |
Hi @majafranz, thank you for this PR/bugfix! Someone from the team will be able to review it shortly :) |
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 opening this PR @majafranz ! I just have one comment, and other than that, this looks great 🎉 .
Also, please ignore the one failing tensorflow test, that is unrelated to this PR and is being addressed.
@Alex-Preciado who do you recommend as a second reviewer for this PR? |
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.
Hi @majafranz! Thank you for fixing this bug, it's a very good catch 🎉
I basically have two comments:
- The test could be reduced a bit regarding its dependency on other parts of the library (QNode and VJP->Jacobian logic). I suggested an alternative, let me know what you think!
- There seems to be a similar bug in the JVP logic. Would you be interested in fixing this as well? It can be taken care of separately, but it also would fit neatly into this PR :)
Co-authored-by: David Wierichs <david.wierichs@xanadu.ai>
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 your contribution, @majafranz 🎉 💯
@majafranz Could you fix the formatting errors preventing this PR from being merged? Thanks! |
be3e44e
to
81dcb26
Compare
Context:
This PR should fix a bug related to computing the gradient of a circuit which supports:
Localised example that fails, without the fix:
Reshaping the jacobian, when computing the VJP does resolve the issue.
Description of the Change:
Jacobian is reshaped according to the shape of dy when computing the VJP for measurements with dimension.
Benefits:
One less bug :)
Possible Drawbacks:
None
Related GitHub Issues:
This PR fixes #5979