-
Notifications
You must be signed in to change notification settings - Fork 575
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
Use qml.math.cast
after qml.math.convert_like
in qml.gradients
to copy dtype
#2120
Conversation
Hello. You may have forgotten to update the changelog!
|
qml.math.cast_like
instead of qml.math.convert_like
in qml.gradients
qml.math.cast
after qml.math.convert_like
in qml.gradients
to copy dtype
Codecov Report
@@ Coverage Diff @@
## master #2120 +/- ##
=======================================
Coverage 99.19% 99.19%
=======================================
Files 228 228
Lines 17476 17485 +9
=======================================
+ Hits 17335 17344 +9
Misses 141 141
Continue to review full report at Codecov.
|
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 ! Only suggestion would be that the test added seems to be more like a workflow test, it might be good to have one or two tests which test the specific lines of code added directly (maybe a direct assertion comparing the dtype of result)
@@ -46,6 +46,7 @@ def compute_vjp(dy, jac, num=None): | |||
|
|||
if not isinstance(dy_row, np.ndarray): | |||
jac = math.convert_like(jac, dy_row) | |||
jac = math.cast(jac, dy_row.dtype) |
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.
very minor @antalszava, but I'm curious why cast_like
wasn't used here, I thought it would be safer that using dy_row.dtype
?
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.
qml.math.cast_like
assumes that the two tensors are already of the same type if I'm not mistaken. I ran into issues using it with Torch, see this commit: 8d84766
[sc-13632] |
Context:
qml.math.cast_like
attempts to copy the dtype of a tensor whereasqml.math.convert_like
only uses the type of the tensors during conversionThe
qml.gradients
module usesqml.math.convert_like
. There may, however, be cases where the dtype would need to be copied, otherwise unexpected behaviour arises.Description of the Change:
Benefits:
Possible Drawbacks:
Related GitHub Issues:
#2105