Pauli exponential support #134
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #134 +/- ##
===========================================
- Coverage 97.67% 83.81% -13.86%
===========================================
Files 5 6 +1
Lines 387 519 +132
===========================================
+ Hits 378 435 +57
- Misses 9 84 +75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…. Finish testing the changes to Evolution operator. Cleanup implementation of ParametrizedEvolution and add further question for code reviewer.
There was a problem hiding this comment.
Thanks for working on this @radumarg . I'm going to do another review soon where I do an even deeper dive into your implementation. Right now, the review mostly covers stylistic and best practices suggestions.
I've also tried to address your concerns about the ParametrizedEvolution implementation, for which I'm also going to get more feedback from the team. Please don't hesitate to reach out for questions 😄
|
Hi @radumarg , could you please share the API docs that show Additionally, I wanted to note that PennyLane has its own decomposition pipeline to decompose circuits into a gate set supported by the device as part of preprocessing. Before this PR, |
|
@mudit2812 the Evolution gate does not seem to work in pennylane 0.41.1 and pennylane-ionq 0.41.0: |
|
@mudit2812 Regarding the documentation for pauliexp gate, Spencer Churchill from IonQ team @splch should be able to respond |
|
@radumarg the preprocessing pipeline gets used under the hood when working with |
|
Hi @radumarg, thanks for opening this PR! 🚀 Could you please add a description to the top of the PR outlining the motivation/context behind the change and what it’s intended to do with a brief description of the changes made? This helps reviewers understand the context, keeps the project history clear, and is essential for us to determine prioritization 🙂. Let me know if you have any questions. |
|
@mudit2812 I have checked and using a circuit with QNode instead of QuantumTape works when jobs are sent to IonQ simulator: |
|
@radumarg would you mind providing some details about the motivation for this PR in the description as well. Do you have any specific workflows that are blocked by the lack of this feature? What are the high-level requirements that you're hoping to accomplish with the work done in this PR? |
|
@mudit2812, @Alex-Preciado I am waiting for some feedback from the IonQ team so that I can provide a better description and context for this PR. Will get back when I have something more concrete. |
|
The primary motivation for adding native Pauli‐exponential (
|
|
@mudit2812, @Alex-Preciado, we have received clarifications from the IonQ team, see above. PR description was updated as well. |
|
Hey @radumarg , I chatted with the team about the open question about With that in mind, I would suggest that we limit the scope of this PR to only add support for For now, feel free to tag me for review after the code relevant to |
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
|
@mudit2812 code rework was performed, what do I need to do in order to see the code test coverage report? |
|
@radumarg the coverage report only gets generated if all the test workflows are successful. Tests are currently failing |
|
@mudit2812 Thanks, the tests were fixed, now I do see test coverage reports, but these reports are not correct. For example, it says:
But those lines are tested by several tests using 'requires_api' argument, which means that the test is sending some circuit to IonQ simulator and it validates the job results. An example of such test is: test_evolution_object_created_from_hamiltonian test. My guess is the test coverage reports do not include test with 'requires_api' . |
@radumarg This is correct. I believe that the Since this PR's content is very non-trivial compared to #132 , it would be great if you can verify that the test coverage is good by running the tests locally. You can do so by adding the following to the EDIT: to run the above command, you need to make sure that the |
There was a problem hiding this comment.
Thanks @radumarg . I've requested just one change about the implementation itself, which is to use trotterization if a user explicitly specifies num_steps != None.
Other than that, I still see opportunity to improve tests via parametrization, but the content of the tests themselves looks great :)
Please leave some details about the inconsistency between the output of the IonQ device vs default.qubit so that the team can investigate. Thanks!
pennylane_ionq/device.py
Outdated
| def _apply_evolution_operation(self, operation, circuit_index, wires): | ||
| """Applies Evolution operations to the internal device state.""" | ||
| name = operation.name | ||
| terms = self._extract_evolution_pauli_terms(operation, wires) | ||
| coefficients = self._extract_evolution_coefficients(operation, wires) | ||
| terms, coefficients = self._remove_trivial_terms(terms, coefficients) | ||
| if len(terms) > 0: | ||
| gate = {"gate": self._operation_map[name]} | ||
| if len(wires) == 2: | ||
| if name in {"SWAP", "XX", "YY", "ZZ", "MS"}: | ||
| # these gates takes two targets | ||
| gate["targets"] = wires | ||
| else: | ||
| gate["control"] = wires[0] | ||
| gate["target"] = wires[1] | ||
| gate["targets"] = wires | ||
| gate["terms"] = terms | ||
| gate["coefficients"] = [-1 * float(v) for v in coefficients] | ||
| gate["time"] = operation.param | ||
| self.input["circuits"][circuit_index]["circuit"].append(gate) |
There was a problem hiding this comment.
In your previous implementation, you were doing the trotter decomposition if num_steps was specified. I think that behaviour still makes sense to me, since the user is explicitly requesting the operation to get decomposed via trotterization.
There was a problem hiding this comment.
After discussing with IonQ team my understanding is that totterization is handled internally by their implementation of pauliexp gates, this is why I removed this feature. I will double-check with @splch and return here with a confirmation if my assumption is correct.
There was a problem hiding this comment.
@splch would you agree with the above?
There was a problem hiding this comment.
So the number of trotter steps specified by the user will never get used?
There was a problem hiding this comment.
hi! yes - pauli_exp already does a single-step product-formula trotterization on the backend, so we don’t need a num_steps option here. if users want more steps or a higher-order rule, they should trotterize the circuit in pennylane before sending it. we can add a docstring note to make this clear
There was a problem hiding this comment.
Update (July 2nd 2025) we are still discussing this issue .. will get back here when we reach a conclusion
There was a problem hiding this comment.
Hey @splch, I had a discussion with the development and product teams, and our concern is the fact that the default behaviour of qml.evolve between PennyLane and IonQ is not the same. This already created a point of confusion during testing for this PR (context), and we suspect it might cause confusion for users of the plugin, as well. I agree that documentation about this is important, but we also have the impression that the default behaviour of the operation should be defined by the frontend, which in this case is PennyLane. I’m happy to continue the discussion about this, as we want to make sure this feature is as polished as possible before merging into master. Our suggestion is to add a warning message that informs the user of the plugin that the default behaviour of qml.evolve changes w.r.t. PennyLane’s.
Tagging @isaacdevlugt, our product manager, for visibility.
There was a problem hiding this comment.
@mudit2812 Here is some feedback from IonQ: IonQ would ideally like to ignore the number of trotter steps and trotterization strategy specified by the user so that we can employ hardware-efficient approximate compilation schemes, e.g., reducing the number of trotter steps given estimations of of the hardware noise map associated with the gate. What we settled on for Qiskit is that we would throw an error if the user sends an undecomposed PauliEvolutionGate to our backend but has not explicitly set a IONQ_RESYNTHESIS environment variable. This error explains that IonQ will be applying its own trotterization/recompilation of the PaulIEvolutionGate, and that if the user does not want this, they should explicitly decompose the circuit to base level gates before sending it to us. So there are two potential solutions:
(1) user-thrown error in cases where an undecomposed PauliEvolutionGate has been sent to IonQ, with the ability to have a user explicitly opt in to our internal compilation/trotterization by declaring an environment variable as currently done in qiskit-ionq packge.
(2) print a warning message that explains that IonQ will apply its own trotterization unless they decompose the gate further beforehand.
Which one would you prefer: (1) that requires the user to set IONQ_RESYNTHESIS env variable in order for Evolution gate to work with IonQ, or (2) that prints a warning to console when a circuit containing Evolution gate is sent to IonQ?
There was a problem hiding this comment.
Hey @radumarg , I discussed with the team, and here are our thoughts:
- In an ideal world, option 1 would be preferred, as it terminates execution before a user runs a potentially incorrect and paid job. But, since there is no public facing documentation for
pauliexpor theIONQ_RESYNTHESISenvironment variable, option 1 does not make sense. - Currently, option 2 seems like the best course of action. We raise a warning if a user uses
qml.evolvethat their specifiednum_stepswill be ignored, and that the backend will automatically choose a compilation scheme. Note that this warning should be raised even ifnum_stepsis not specified, since in base PennyLane,num_steps=Noneis equivalent to an exact time evolution, not an approximation. Along with the warning, this behaviour should be documented in the docstring of the device being updated.
Let me know if you have any questions. @isaacdevlugt feel free to chime in if I missed anything.
There was a problem hiding this comment.
Yep @mudit2812 covered everything 💯!
|
@mudit2812 I have implemented most of your suggestions, increased code coverage, which should now be complete for the code modified by this ticket. I have made some comments and added some sample Qiskit code above on the issue of Pennylane apparently not returning correct simulation results. Can you please have a look with your team and let me know what you find out? |
|
@mudit2812 did you have the time to test the incorrect simulation results documented above? Should we wait for you to complete your investigation, or we can proceed with this ticket? |
|
@mudit2812 I left a question on the only issue with is not resolved yet (see above). After consulting with IonQ team I explained why IonQ does not implement number of steps for Troterrization and suggested to solutions in order to warn the user about this nonstandard behavior. Please take a look and let me know which one you prefer. Thanks! |
The primary motivation for adding native Pauli‐exponential (
pauliexp) support is performance - when we sendpauliexpdirectly to IonQ, our compiler can optimize across a larger context (instead of first Trotterizing), which speeds up development and execution times. No existing workflows are strictly blocked, but allowingpauliexpto go through "as-is" reduces compilation overhead and yields shorter circuits.pauliexpisn’t yet documented publicly, we already support it. The qiskit‐ionq package already includes helpers for Pauli exponentials (Qiskit‐IonQ helpers, so the hardware/runtime will accept it natively.