Correctly enforce matrix size check in CommutationChecker#15933
Correctly enforce matrix size check in CommutationChecker#15933alexanderivrii merged 2 commits intoQiskit:mainfrom
CommutationChecker#15933Conversation
… checker. A typo in the matrix size check allowed to sneak into a matrix-based check even if the operations exceeded the maximum matrix size. The operations `first_..` and `second_..` are sorted, but `..1` and `..2` are not! The wrong one was used here.
Cryoris
added
stable backport potential
|
One or more of the following people are relevant to this code:
|
|
|
||
| def test_large_custom_gate(self): | ||
| """Test a large custom gate is caught by the qubit threshold.""" | ||
| big = UnitaryGate(np.eye(2**12)) |
There was a problem hiding this comment.
This may be a little unnecessarily large for the test - I think it ends up allocating 384MiB in matrices lol. Can we just use 6 qubits or so, or is 12 the threshold?
There was a problem hiding this comment.
It still panics with 9 qubits, that should be 384/8 so 48ish MiB? Maybe still a bit large? 😅
There was a problem hiding this comment.
We can pass matrix_max_num_qubits to scc.commute, thus allowing to reduce the test size.
There was a problem hiding this comment.
I was testing a path that would panic in the existing code, which was happening because the matrix sizes get too large. I can also reduce it and ensure the commutation evaluates to False (since it should not enter the check) when it actually comes out to True (if you were doing the check, which you shouldn't).
There was a problem hiding this comment.
I'd use the smallest size that still tests what you want. 9 qubits is actually 64x smaller - you're thinking about statevectors, but using operators.
I mean, even the big big matrices probably wouldn't be a problem, they were just a bit unnecessary haha
There was a problem hiding this comment.
I changed to the True/False setting I described above, which still tests the right thing but doesn't rely on triggering a panic anymore 🙂 So that only needs a 4-dim identity now 😄
This test now doesn't rely on the internal path panicking, but still tests the right behavior since the results with and without a matrix check differ.
alexanderivrii
left a comment
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks Julien, good to have this fixed.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* Fixed the matrix size check for non-cachable gates in the commutation checker. A typo in the matrix size check allowed to sneak into a matrix-based check even if the operations exceeded the maximum matrix size. The operations `first_..` and `second_..` are sorted, but `..1` and `..2` are not! The wrong one was used here. * Reduce test size This test now doesn't rely on the internal path panicking, but still tests the right behavior since the results with and without a matrix check differ. (cherry picked from commit b43a2f3)
4 participants
Fixed the matrix size check for non-cachable gates in the commutation checker.
A typo in the matrix size check allowed to sneak into a matrix-based check even if the operations exceeded the maximum matrix size. The operations
first_..andsecond_..are sorted, but..1and..2are not! The wrong one was used here.