PennyLaneAI / catalyst

A JIT compiler for hybrid quantum programs in PennyLane
https://docs.pennylane.ai/projects/catalyst
Apache License 2.0
131 stars 29 forks source link

Consider eliminating redundant "shots" parameters in device back end API #781

Open WrathfulSpatula opened 4 months ago

WrathfulSpatula commented 4 months ago

Issue description

As a general suggestion on your (C++ device back end) API, in Catalyst::Runtime::QuantumDevice, as someone implementing a simulator device back end over at pennylane-qrack, I notice that you require a "shots" parameter in certain methods that is redundant with the dimension required for DataView instances. Immediately at the top of such methods, you make sure that shots is exactly the same as the size of the DataView instance, and you raise exception if the two do not match.

Given that the DataView must already have the same size as the shots integer, why not remove the shots parameter and simply consider it implied by the size of the DataView? This way, there's never a case where it's necessary to reject on input validation.

Source code and tracebacks

For example,

void Sample(DataView<double, 2> &samples, size_t shots) override
{
    RT_FAIL_IF(samples.size() != shots, "Invalid size for the pre-allocated samples");
    [...]
}

Instead, the signature could simply be like this:

void Sample(DataView<double, 2> &samples) override
{
    // "shots" parameter value is implied by just "samples.size()."
    [...]
}
dime10 commented 4 months ago

Thank you for the feedback!

I agree the assertions are somewhat redundant, although I think removing the explicit shots parameter from the api functions would it make the api slighter more complicated. Somewhere (that is not code) we would have to document how to obtain the number of shots from the data buffer size, which are not necessarily identical. For samples for example the total number of produced elements in the buffer will be n_wires * n_shots. For counts the data buffers are of size 2 ** n_wires.