limiting matrix-based commutativity check #10495
Conversation
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 5656018131
💛 - Coveralls |
There was a problem hiding this comment.
This looks like a reasonable limit to me, and one that's very unlikely to ever be tripped (the bug report from a stress test notwithstanding), since I'm not aware of much hardware that has support for 4+q operations natively (although I suppose the MS interaction in ions can do that).
Looks like lint's complaining a bit - you can probably safely disable too-many-returns in the function that's tripping it, if you think that combining some of the early returns will make the function harder to understand not easier. The line length thing I think is in the docstring, which black won't touch, so you'll need to edit it manually.
There was a problem hiding this comment.
The code looks good now, thanks. A reason I'm slightly hesitant to tag this for backport to 0.25.0 is that strictly I think this is more of a new feature, and the bugfix is very low priority. Perhaps we should (in addition to the fix note) put a feature note in about the new option to CommutationChecker, and merge this for the October Qiskit release instead? What do you think, Sasha?
There was a problem hiding this comment.
Thanks for the quick updates!
For context: Sasha and I spoke privately and agreed that this would be safer as a feature note targetting Qiskit 0.45 rather than the stable 0.25.0 branch, because of the potential changed behaviour. The bugfix is low priority, since it will only trigger with particular very large circuits when no basis gates are set (or a backend that claims to support arbitrary-sized mcx/mct operations).
Summary
Fixes #10488.
The matrix-based commutativity check in
CommutationCheckerrequires to construct2^N x 2^Nmatrices, leading to memory problems whenNis large.I have added a new argument
max_num_qubitstoCommutationChecker::commutethat allows to skip the commutativity check if the number of qubits of either operation exceeds this amount.Note that simpler versions of the commutativity check not involving matrix multiplication continue to work as before (e.g., two quantum operations over disjoint sets of qubits commute regardless of the number of qubits).
Details and comments
This actually changes the behavior of commutation checking when the number of qubits is in the range from 4 to 12 (when the matrices could be constructed and multiplied). However personally I don't think we will be missing a lot of optimization opportunities. I am even thinking that we should maybe limit
max_num_qubitsto 2, what are your thoughts on this?Implementation-wise, I was a bit unsure whether to make
max_num_qubitsto be a global constant, or to pass it as the argument toCommutationChecker::__init__, or to pass it as the argument toCommutationChecker::commute(this is what I have chosen at the moment). I can see a potential use-case of having the same instance ofCommutationChecker, and then first doing some optimizations based on a lighter-weight version of the check, and then possibly more optimizations based on heavier-weight version of the check (the same instance allows to use the cached results from previous calls).