[WIP] Set shots #1784
[WIP] Set shots #1784
Conversation
|
Hello. You may have forgotten to update the changelog!
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1784 +/- ##
==========================================
- Coverage 96.60% 96.54% -0.07%
==========================================
Files 82 82
Lines 9211 9224 +13
Branches 872 878 +6
==========================================
+ Hits 8898 8905 +7
- Misses 254 255 +1
- Partials 59 64 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@paul0403 It's still far from being completed but I just want to know whether it's a good idea to directly modify the |
| @@ -1328,6 +1328,31 @@ def trace_quantum_function( | |||
| out_tree: PyTree shapen of the result | |||
| """ | |||
|
|
|||
| if qml.transforms.set_shots in qnode.transform_program: | |||
There was a problem hiding this comment.
I just want to know whether it's a good idea to directly modify the device._shots passed to catalyst.
I would say setting the shots on the QJITDevice like you're doing is reasonable. Although I might suggest moving this code to the QFunc class in catalyst/qfunc.py, since it's not really related to tracing and this module is already fairly beefy.
There was a problem hiding this comment.
Why are we concerned about the device at all? Isn't the point of the PL work to decouple the shots from the device? 😅 Sorry if I'm missing something here @JerryChen97
| @@ -1328,6 +1328,31 @@ def trace_quantum_function( | |||
| out_tree: PyTree shapen of the result | |||
| """ | |||
|
|
|||
| if qml.transforms.set_shots in qnode.transform_program: | |||
There was a problem hiding this comment.
One thing I don't fully understand is what difference this approach makes, since the information is still static if it is in the qnode program. That is, the user is not able to set different shot values when calling their function, right?
I guess with this the user can set shots on a per qnode level, rather than a per device level?
There was a problem hiding this comment.
This is exactly what I thought as well! I don't expect this direct mutation to device to be the final solution at all. Just curious about any potential risk if this device mutation happens
There was a problem hiding this comment.
No risk I think because each qnode triggers the creation of a new QJITDevice, the scoping there matches.
| @@ -16,7 +16,7 @@ enzyme=v0.0.149 | |||
|
|
|||
| # For a custom PL version, update the package version here and at | |||
| # 'doc/requirements.txt | |||
| pennylane=0.42.0-dev33 | |||
There was a problem hiding this comment.
No longer needed, we updated to 48 (#1795 )
| user_transform = qnode.transform_program | ||
| set_shots_transform = TransformProgram() |
There was a problem hiding this comment.
Wait, is the new set_shots being implemented as an entire transform in pennylane? 😨
There was a problem hiding this comment.
Would this be a big issue for Catalyst?
There was a problem hiding this comment.
I'm not sure. I was just hoping that we can replace the shots = get_device_shots(device) in this jax tracer file for however PL handles it now on the qnode...
There was a problem hiding this comment.
Would this be a big issue for Catalyst?
@paul0403 Maybe you can get an overview of how the core team plans to handle shots going forward, that could be helpful.
But for us currently, I would say no, there is no issue with the proposed change. Whether it's the most sensible option going forward might be a different question.
There was a problem hiding this comment.
On PL side our approach of how to decouple the shots from devices is also under changes. There might be a better way to do here as well. I'll update asap it's done!
Context:
Make non-plxpr qjit compatible with new
qml.set_shotsscheme:shotsinformation will be decoupled fromdeviceDescription of the Change:
set_shotsbeforecatalyststarts to consumedeviceandqnodeBenefits:
Possible Drawbacks:
Related GitHub Issues:
[sc-90929]