Enable multi circuit submission

[radumarg]

Enable batch circuit submit: adding batch_execute method(circuits_array) as an alternative to execute(circuit) method.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 50.19455% with 128 lines in your changes missing coverage. Please review.

Project coverage is 74.64%. Comparing base (267d34e) to head (78aca8d).

Files	Patch %	Lines
pennylane_ionq/device.py	50.00%	128 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #111 +/- ## =========================================== - Coverage 96.92% 74.64% -22.29% =========================================== Files 5 5 Lines 325 556 +231 =========================================== + Hits 315 415 +100 - Misses 10 141 +131 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[isaacdevlugt]

Hey @radumarg! Apologies for the wait. @Shiro-Raven is on the case 🕵️!

[radumarg]

Hi Ahmed,

Thank you for the review comments on Pennylane-Ionq changes. Here is what I did:

(1) As you suggested, I have expanded and corrected the docstrings for the newly added methods. (2) I have followed your suggestion of delegating the calculations to QubitDevice to reduce code duplication, w.r.t. the Pennylane code base. Unfortunately, I was not able to remove all code duplication. I am fully aware this is a maintenance problem but could not find a better solution, the code is very tightly coupled. If you have other suggestions that would help reduce code duplication even more, I will be more than happy to implement those changes.

Thank you and let me know if you have additional comments.

Regards, Radu

On Tue, Jul 23, 2024 at 7:15 PM Ahmed Darwish @.***> wrote:

@.**** requested changes on this pull request.

Hello @radumarg https://github.com/radumarg! Thank you for your contribution and patience while we reviewed your PR! I have a couple of comments regarding the implementation you decided to go with that I think should be addressed before merging your work. I am going to list them in order of increasing criticality:

1. I see multiple spelling mistakes in the comments, for example QubiDevice instead of QubitDevice. In addition, a lot of the docstrings for the custom measurement functions are missing explanation for the circuit_index argument.

2. I see that IonQ's API has support for two different keys for circuit submission, one named circuit and the other called circuits, and I see that you have adapted this in your code, and also included new class attributes, self.histograms and self._samples_array, to store the output results of the different submitted jobs. My thoughts about this are:

   • It is much simpler to just maintain the plural version of the key circuits, for easier maintenance and to avoid the need to maintain if-else statements for the different scenarios.
   • The same goes for self._samples, which we can safely delete now since it's a private attribute, and adapt to the usage of self._samples_array solely.
   • I would've preferred to do the same for self.histogram, but I think it's not worth the breaking change.

3. I find the way you have included variations of statistics, probs, _measure, and the other measurement functions problematic, for the following reasons:

   • Copy-pasting functions, especially if they are critical like those, adds on a huge maintenance overhead to the developer, as they would need to remember updating them if the original functions get updated.
   • To provide an idea of what you can do, this is how I would delegate the calculations to QubitDevice.estimate_probability:

def estimate_probability( self, wires=None, shot_range=None, bin_size=None, circuit_index=None ) : circuit_index = circuit_index or 0 self._samples = self._samples_array[circuit_index] res = super().estimate_probability(wires, shot_range, bin_size)

Reset attribute

self._samples = None
return res

I am aware that this contradicts my suggestion from before about removing self._samples in the previous point, so feel free to ignore that and reconcile my suggestions as you see fit 😄

• On a technical note, your comments of these functions mention that they are "overwriting"/"overriding" the ones from QubitDevice. Please note that this is inaccurate, since an overridden function must take the exact same input arguments as the one from the parent class. What you have done here is known as "overloading" due to the addition of the circuit_index argument. Let me know if you have any questions, and thanks again for your contribution!

— Reply to this email directly, view it on GitHub https://github.com/PennyLaneAI/PennyLane-IonQ/pull/111#pullrequestreview-2194446757, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2UFTH2R5YJA3CP55LXAD3ZNZ6QLAVCNFSM6AAAAABJPYR2M6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJUGQ2DMNZVG4 . You are receiving this because you were mentioned.Message ID: @.***>

[Shiro-Raven]

@radumarg Thanks for the changes! I am gonna look again at the duplicated methods and see if it's possible to delegate them to the parent class, in the meantime, could you address the test coverage issues along with the codefactor and docs build issues? Let me know if you are not familiar with these tools and I can give you some pointers. You can always check the logs and error messages by clicking on Details in the workflows list.

[radumarg]

@Shiro-Raven I have taken a look over the code coverage. Testing all new lines in a method like statistics() in IonQDevice(QubitDevice) is insane. What I did in my change was to create a lot of code duplication w.r.t. Pennylane base code, I fully understand that. Now testing those changes will not only be a lot of work but will create more duplication not only in production code but in testing code as well. This is not practical. I am trying to figure out how to avoid overloading the statistics() method, but nothing I can think of looks decent enough. Meanwhile, if you have any suggestion, please let me know.

[radumarg]

@Shiro-Raven Following up my previous comment: I am afraid there is no other solution to the many problems in this code change than to create a new branch and attempt to come up with a new design. As mentioned above, I will appreciate any suggestions you might have. Thanks!

[radumarg]

I am afraid there is no other solution to the many problems in this code change than to create a new branch and attempt to come up with a new design.

[Shiro-Raven]

Hello again @radumarg! I apologize for the late reply. I just wanted to carry out my due diligence and verify that indeed there is no way to adapt the changes to the logic you are trying to add. I think the main goal of your PR is still valid. Submitting several circuits to the cloud service would definitely be beneficial and more efficient. I believe one way to go about this is to limit our scope to simply extending the functions of the plug-in to support this, without necessarily overriding any functions from the parent class. What I am thinking of is a modification to the submit_job function that would accept a batch of circuits and submit them, and later on keeping all of their results in a self._histograms object similar to the way you have been handling it. I believe this would give you more time to think about a good way to integrate your initial idea with how the QubitDevice class handles execution results. The team is aware of our discussion and we are actually grateful for your PR! It brought our attention to a blind spot that we were not aware of before.

[radumarg]

@Shiro-Raven I have created a new implementation here: https://github.com/radumarg/PennyLane-IonQ/tree/multi-circuit-submission-2 The average user will use batch submit as any user would do: results = dev.batch_execute([tape1, tape2]) than extract the result from the results array and will not notice there is a problem under the hood. A more ambitious user that wants to use some functions under the hood like: dev.sample(qml.PauliZ(0)) method will have to do this first : "dev.set_current_circuit_index(0)" then "results0 = dev.sample(qml.PauliZ(0))" This is obviously not very good design but will achieve our purpose here which is to implement batch circuit submit without any code duplication w.r.t. Pennylane base code. I propose we use this implementation (a pull request will be created soon) and once you guys modify the Pennylane baseline code we can update our implementation.

[radumarg]

@Shiro-Raven I did not yet receive feedback for my new design proposal from IonQ team due to August holidays. I might have to wait until the beginning of September. In any case, I would say implementing your proposal makes sense regardless. The plan was for me to implement the changes as I suggested and later switch to using the new submit_job you mentioned. Do you think this is something that you could address soon?

[Shiro-Raven]

@radumarg Apologies again for the late reply. The new implementation looks good! I have a couple of cosmetic comments that I can mention once you create a PR, but I think this is already a huge improvement over the previous implementation.

[radumarg]

@Shiro-Raven There is a new pull request with the improved solution: https://github.com/PennyLaneAI/PennyLane-IonQ/pull/121