Oxidize commutation checker #12870
Merged
Oxidize commutation checker #12870
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
One or more of the following people are relevant to this code:
|
|
Do you already have some profilings for this? 🚀 |
|
No :-( The part responsible for composing the commutator by using |
|
One or more of the following people are relevant to this code:
|
|
Ready for review, I'm running benchmarks later. :-) |
This commit removes the pending deprecation decorator from the python class definition as the Python class just internally is using the rust implementation now. This also removes directly using the rust implementation for the standard commutation library global as using the python class is exactly the same now. We can revisit if there is anything we want to deprecate and remove in 2.0 in a follow up PR. Personally, I think the cache management methods are all we really want to remove as the cache should be an internal implementation detail and not part of the public interface.
This commit makes the pickling of cache entries explicit. Previously it was relying on conversion traits which hid some of the complexity but this uses a pair of conversion functions instead.
Now that the python commutation checker and the rust commutation checker are the same thing the ddt parameterization of the commutation checker tests was unecessary duplication. This commit removes the ddt usage to restore having a single run of all the tests.
mtreinish
reviewed
Aug 30, 2024
There was a problem hiding this comment.
Overall I'm happy enough with the current state of the PR to move forward here. But since I've touched a fair amount of this code now I don't think that I should approve it anymore. I think there are probably some things we could cleanup or improve in follow ups but nothing worth blocking over in particular. Especially considering this is more of an incremental step to enable #12995.
|
Hmm, it looks like there might be a logic bug in the new code somewhere and it's not cancelling something that it used to. I ran a quick asv run to see the speedup (which is looking pretty good) but the depth for the qft example has regressed: |
raynelfss
reviewed
Aug 30, 2024
There was a problem hiding this comment.
Thank you for working on this @Cryoris, I just have a couple of inline comments, mostly just speaking about a lot of precedures that could panic! and could be handled with a Result or PyResult return value. Some of these already return PyResult so it wouldn't be too hard to just make those work and return Python error types.
The reason why we should prefer PyResults over panic! for python exposed API, is that Python cannot serialize PanicException instances, thus these are taken with more severity and can't be handled properly. Now, this is only for python exposed API, for rust based api's it should be fine as long as the panic! is produced due to a programming error in rust and not necessarily something coming from Python.
That could maybe point to different handling of gates with minuscule rotation angles -- maybe the old code considered things as commuting that the new one does not. We can check by looking at gates with tiny angles on both branches... Edit: Yes that indeed differs. For example: |
Co-authored-by: Raynel Sanchez <raynelfss@hotmail.com>
|
9235004 adds backward compatible handling of the tolerances, and some further error propagation instead of eager unwrapping (in spirit of @raynelfss previous comments). If CI passes, I think it should be good to go 🙂 |
such as the relative placement and the parameter hash
6 tasks
|
I reran asv after @Cryoris's recent changes and the performance is looking quite good: |
raynelfss
approved these changes
Sep 3, 2024
There was a problem hiding this comment.
Thank you @sbrandhsn and @Cryoris for working on this. LGTM!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
fixes #12702
Details and comments
This PR implements the commutation checker with a commutation cache and library as well as a helper function for composing unitary matrices in Rust. I left a couple of things for future work:
Operator.composebut can be generalized fairly easily whenOperatoris implemented in Rust, too.DAGOpNodeare aPyTupleand I wanted to get rid of Python objects as early as possible. :-) The signatures can be shortened when qubits and clbits are also in Rust space.Two things are changed lower in the Qiskit stack in
circuit_instruction.rsandoperations.rs:is_conditionedreturns whether an instruction is conditioned on classical bits and has been added tocircuit_instruction.rsThe- removed after further discussionto_matrixmethod ofPyGateandPyOperationhas been extended to include a call to python-sideOperator(self)in case the python-sideto_matrixcall fails. This is a change in behavior and I can pull up that code into thecommutation_checker.rsbut I wanted to hear everyone's opinion here. I think that if we have all the necessary information to return a matrix, we should attempt to do that even if it comes at an increase in runtime.I'm fairly unfamiliar with Rust so if some of the behavior can be describe more succinctly please let me know. :-)
Speedups
We can see a speedup of up to 5x depending on the type of input circuit that should be evaluated for commutation relations. I suspect that we do not see a 10x speedup, as some may have expected, because: the heavy-lifting (matrix multiplications) is done by high-performance numpy code, the incorporated commutation caching quickly turns most commutation resolutions into a constant-time lookup the larger the circuit becomes, and the commutation library already contains most of the gates that we have in rust.
Random circuits with parameterized gates only benefit the most from the rust implementation:
Followed by random circuits with any kind of gate:
The speedup becomes larger for Clifford circuits on larger circuits as gate checks outweigh the contant-time lookup of clifford gates in the commutation library by far
