TmpPauliRot still decomposes if the theta of zero is trainable #4585
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #4585 +/- ##
=======================================
Coverage 99.64% 99.64%
=======================================
Files 375 375
Lines 33358 33358
=======================================
Hits 33239 33239
Misses 119 119
☔ View full report in Codecov by Sentry. |
|
[sc-44387] |
There was a problem hiding this comment.
The change seems technically fine but I don't have enough knowledge about SU to know where it has an impact, I let @dwierichs or @Qottmann to accept it first.
|
Simply removing it is a bit problematic. That is because without this custom decomposition the pulse gradients always get 4**n-2 unnecessary, empty rotation gates. This slows down simulation and generates additional noise on real device execution (if not transpiled away, which currently is not the case AFAIK) |
…aneAI/pennylane into special-unitary-works-dq2
timmysilv
changed the title
|
I have updated the changes and PR description. I'm not convinced this is really better, but it's clearer. Lmk thoughts |
**Context:** While porting DQ2, the integration tests failed. It took me waaaaay long to find out why, but it's because DQL kept the `TmpPauliRot` ops around despite their value originally being 0, while DQ2 did away with them, and that broke differentiation. I figure this was supposed to be an optimization, but it's breaking assumptions made by DQ2. The break is because DQL reaches [jax's trainable-parameter evaluation code](https://github.com/PennyLaneAI/pennylane/blob/cbc8d19e6a36f0d4d38b44eabd4a8b98db677327/pennylane/interfaces/jax.py#L121-L126) _before_ filtering out the zeros, thereby enabling differentiation over those zeros, while DQ2 reaches that code _after_ filtering them out (so it has no trainable parameters and doesn't differentiate over anything). The filtering logic assumed DQL's execution order, and the slight re-arrangement of when we decompose unsupported ops broke this with DQ2. **Description of the Change:** `TmpPauliRot.compute_decomposition` does not become nothing if the theta value is trainable. **Benefits:** `SpecialUnitary` differentiation works with DQ2 **Possible Drawbacks:** I feel like this still might hurt performance, but this is a consequence of `SpecialUnitary`-implementation-meets-execution-pipeline assumptions. Fixing that requires a deeper change, and it should probably happen in `SpecialUnitary`'s code. --------- Co-authored-by: Romain Moyard <rmoyard@gmail.com>
|
Very neat solution, this. :) @timmysilv edit: Unfortunately, this incurs a performance digression that can not be solved easily. |
Context:
While porting DQ2, the integration tests failed. It took me waaaaay long to find out why, but it's because DQL kept the
TmpPauliRotops around despite their value originally being 0, while DQ2 did away with them, and that broke differentiation. I figure this was supposed to be an optimization, but it's breaking assumptions made by DQ2.The break is because DQL reaches jax's trainable-parameter evaluation code before filtering out the zeros, thereby enabling differentiation over those zeros, while DQ2 reaches that code after filtering them out (so it has no trainable parameters and doesn't differentiate over anything). The filtering logic assumed DQL's execution order, and the slight re-arrangement of when we decompose unsupported ops broke this with DQ2.
Description of the Change:
TmpPauliRot.compute_decompositiondoes not become nothing if the theta value is trainable.Benefits:
SpecialUnitarydifferentiation works with DQ2Possible Drawbacks:
I feel like this still might hurt performance, but this is a consequence of
SpecialUnitary-implementation-meets-execution-pipeline assumptions. Fixing that requires a deeper change, and it should probably happen inSpecialUnitary's code.