First-order finite-diff differentiation with float32 parameters #1381
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1381 +/- ##
=======================================
Coverage 98.16% 98.16%
=======================================
Files 154 154
Lines 11558 11559 +1
=======================================
+ Hits 11346 11347 +1
Misses 212 212
Continue to review full report at Codecov.
|
albi3ro
added
the
review-ready 👌
There was a problem hiding this comment.
Thanks @albi3ro! PR looks good from my end. Only suggestion: I recommend adding a test that passes under this fix, but would have failed previously, just to ensure this remains fixed in the future.
We should have a better method to prevent trainable parameters from ever being non-scalers than just discouraging it.
Agree - I think the correct approach here is requires_grad=False by default, as it requires users to deliberately turn on differentiation. However, this is a big change.
| @@ -956,3 +956,31 @@ def circuit(a): | |||
| res = circuit(0.8, shots=2) | |||
| assert len(res) == 2 | |||
| assert dev.shots == 3 | |||
|
|
|||
|
|
|||
| def test_finitediff_float32(tol): | |||
There was a problem hiding this comment.
I was unsure of where to put this test. It's not actually an interface specific thing, just a float32 thing.
There was a problem hiding this comment.
Also, I don't know why StronglyEntanglingLayers has a problem but simpler circuits don't.
There was a problem hiding this comment.
I was unsure of where to put this test. It's not actually an interface specific thing, just a float32 thing.
It looks fine here :) Alternatively, it could also be placed in test_jacobian_tape (does a file like that exist?).
Also, I don't know why StronglyEntanglingLayers has a problem but simpler circuits don't.
Weird 🤔
Thanks @albi3ro! So just to confirm, this test fails on master?
There was a problem hiding this comment.
test fails on master.
I tried writing it in terms of tapes, but then you have to expand StronglyEntanglingLayers and update parameters and set them trainable, so I just went the simpler route of testing at QNode level.
| @@ -956,3 +956,31 @@ def circuit(a): | |||
| res = circuit(0.8, shots=2) | |||
| assert len(res) == 2 | |||
| assert dev.shots == 3 | |||
|
|
|||
|
|
|||
| def test_finitediff_float32(tol): | |||
There was a problem hiding this comment.
I was unsure of where to put this test. It's not actually an interface specific thing, just a float32 thing.
It looks fine here :) Alternatively, it could also be placed in test_jacobian_tape (does a file like that exist?).
Also, I don't know why StronglyEntanglingLayers has a problem but simpler circuits don't.
Weird 🤔
Thanks @albi3ro! So just to confirm, this test fails on master?
Fixes #1375
Currently,
float32parameters, like those default in the JAX interface, might experience floating-point errors withdiff_method='finite-diff'andorder=1.JacobianTape.numeric_pdconverts all parameters tofloat64, but the calculation of"y0"does not have such a conversion.Downside:
We assume that the tape parameters are all scalers. If matrix gate parameters, like with
qml.Hermitianorqml.QubitUnitary, are not set to untrainable, then this fix will give errors. We should have a better method to prevent trainable parameters from ever being non-scalers than just discouraging it.