Use numpy.linalg.det instead of scipy's function #9179
Conversation
This commit updates the det() function used for computing the determinant of the unitary used as part of the calculation of one and two qubit unitary deocmposers. It is now using the numpy linalg module's det() function. Previously it was using the function from scipy which required a runtime import and also marginally slower under cProfile (which might be a red herring because of profiler overhead as scipy has more python space calls). This also standardizes the det() call usage in the module as other places were already using the numpy version of the function.
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
There was a problem hiding this comment.
At least historically, the recommendation was to use scipy.linalg instead of numpy.linalg because Scipy would guarantee that all its methods were built against some BLAS/LAPACK implementation, whereas Numpy would sometimes use custom fallbacks if it didn't have BLAS/LAPACK available at build. I don't really know what the status of that is any more (my knowledge is about 5 years old at this point) - the Scipy documentation still spouts the same reasoning, but I don't really know if it's up-to-date.
That aside, in principle this swap seems acceptable to me.
Pull Request Test Coverage Report for Build 3524760087
💛 - Coveralls |
|
I guess I could see that as being a potential pitfall. At least on our supported platforms there are numpy wheels available (except ppc64le and s390x) which include openblas afaik. Looking through the numpy repo there is still a In this case though I'm not sure there is really any harm we were already using a mix of numpy and scipy's |
|
Yeah, I'm happy just to merge this. |
This commit ports the per basis parameter calculation from python to rust. In looking at the runtime performance regression caused by Qiskit#8917 the majority is just that we're doing more work synthesizing to more available basis to potentially produce better quality results. Profiling the transpiler pass shows we're spending a non-trivial amount of time in numpy/scipy (depending on whether it's before or after Qiskit#9179) computing the determinant of the unitary. This is likely because those determinant functions are designed to work with an arbitrarily large square matrix while for the 1 qubit decomposer we're only ever working with a 2x2. To remove this overhead this commit writes a dedicated rust function to compute the determinant of a 2x2 complex matrix and then also adds dedicated functions to calculate the angles for given basis to rust as we can easily just return the end result from rust. Related Qiskit#8774
This commit ports the per basis parameter calculation from python to rust. In looking at the runtime performance regression caused by Qiskit#8917 the majority is just that we're doing more work synthesizing to more available basis to potentially produce better quality results. Profiling the transpiler pass shows we're spending a non-trivial amount of time in numpy/scipy (depending on whether it's before or after Qiskit#9179) computing the determinant of the unitary. This is likely because those determinant functions are designed to work with an arbitrarily large square matrix while for the 1 qubit decomposer we're only ever working with a 2x2. To remove this overhead this commit writes a dedicated rust function to compute the determinant of a 2x2 complex matrix and then also adds dedicated functions to calculate the angles for given basis to rust as we can easily just return the end result from rust. Related Qiskit#8774
* Oxidize parameter calculation for OneQubitEulerDecomposer This commit ports the per basis parameter calculation from python to rust. In looking at the runtime performance regression caused by #8917 the majority is just that we're doing more work synthesizing to more available basis to potentially produce better quality results. Profiling the transpiler pass shows we're spending a non-trivial amount of time in numpy/scipy (depending on whether it's before or after #9179) computing the determinant of the unitary. This is likely because those determinant functions are designed to work with an arbitrarily large square matrix while for the 1 qubit decomposer we're only ever working with a 2x2. To remove this overhead this commit writes a dedicated rust function to compute the determinant of a 2x2 complex matrix and then also adds dedicated functions to calculate the angles for given basis to rust as we can easily just return the end result from rust. Related #8774 * Eliminate python function for staticmethod definition This commit removes one layer of function calls for the OneQubitEulerDecomposer's staticmethods for calculating parameters. Previously, there was a python function which called an inner rust function, this eliminates one layer and attaches the rust function directly as a static method to the class definition.
This commit updates the det() function used for computing the determinant of the unitary used as part of the calculation of one and two qubit unitary deocmposers. It is now using the numpy linalg module's det() function. Previously it was using the function from scipy which required a runtime import and also marginally slower under cProfile (which might be a red herring because of profiler overhead as scipy has more python space calls). This also standardizes the det() call usage in the module as other places were already using the numpy version of the function.
* Oxidize parameter calculation for OneQubitEulerDecomposer This commit ports the per basis parameter calculation from python to rust. In looking at the runtime performance regression caused by Qiskit#8917 the majority is just that we're doing more work synthesizing to more available basis to potentially produce better quality results. Profiling the transpiler pass shows we're spending a non-trivial amount of time in numpy/scipy (depending on whether it's before or after Qiskit#9179) computing the determinant of the unitary. This is likely because those determinant functions are designed to work with an arbitrarily large square matrix while for the 1 qubit decomposer we're only ever working with a 2x2. To remove this overhead this commit writes a dedicated rust function to compute the determinant of a 2x2 complex matrix and then also adds dedicated functions to calculate the angles for given basis to rust as we can easily just return the end result from rust. Related Qiskit#8774 * Eliminate python function for staticmethod definition This commit removes one layer of function calls for the OneQubitEulerDecomposer's staticmethods for calculating parameters. Previously, there was a python function which called an inner rust function, this eliminates one layer and attaches the rust function directly as a static method to the class definition.
Summary
This commit updates the det() function used for computing the determinant of the unitary used as part of the calculation of one and two qubit unitary deocmposers. It is now using the numpy linalg module's det() function. Previously it was using the function from scipy which required a runtime import and also marginally slower under cProfile (which might be a red herring because of profiler overhead as scipy has more python space calls). This also standardizes the det() call usage in the module as other places were already using the numpy version of the function.
Details and comments