-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Operator apply permutation #9403
Operator apply permutation #9403
Conversation
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:
|
Here are some fun numbers on the efficiency of the approach. Here for each number of qubits
|
Pull Request Test Coverage Report for Build 4747761829
💛 - Coveralls |
Here are a few questions. The function Update: here is a tutorial on Operators that explains about subsystems and gives an example of an Operator representing a system with one qubit and one qutrit: https://qiskit.org/documentation/tutorials/circuits_advanced/02_operators_overview.html |
let's aim for consistency with this PR which implemented |
This PR proposes to calculate I sometimes would like to calculate the conjugate action |
Ikko: why do you say it's not a good idea? Sure, it's more efficient to do on a statevector, but in the case of permutations it's not a terrible difference, since we can just apply a row/column-reordering rather than doing matrix-matrix multiplication. The motivation here for allowing one-sided permutations on |
So, the current code only works for The current code is more naive. For each row/column index from 0 to 2^n, we compute where it would move after applying the permutation, by first converting it to a bitstream: Note (*): this is a perfect opportunity to cite the paper by Georg Cantor Über einfache zahlensysteme. Zeitschrift für Mathematik und Physik, 14:121–128, 1869 (note the year!), and yes it's in German. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I didn't know that use case. Then I think it is a good idea.
I tend to think of operators as observables or density matrices, so forgot about unitary operators. So I said conjugate action is better, but for the transpiler this proposal is nice.
Thank you. Almost LGTM! Could you add the release note?
@@ -198,6 +198,67 @@ def from_label(cls, label): | |||
op = op.compose(label_mats[char], qargs=[qubit]) | |||
return op | |||
|
|||
def apply_permutation(self, pattern: list, front: bool = False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about inlace
option? Or if this is destructive method, it would be nice to return nothing. If this is non-destructive, copy is needed in the first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this probably should act on a copy - that seems to be in line with what the rest of the Operator
methods do, and they don't have inplace
options. If we need inplace=True
versions of things in the future, we might want to revisit on a larger scale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I have reimplemented the functionality based on the suggestions; there is no longer a need for the inplace
option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the method I've sketched out does turn out to be the correct way to handle arbitrary-dimensioned tensors (I think it does work, at least up to the limit that Numpy only allows 31 dimensions in its arrays, so we can only have 15 spaces...), then we'd also need to add a couple of tests of that.
op_pat = [0] * (2**nq) | ||
for r in range(2**nq): | ||
# convert row to bitstring, reverse, apply permutation pattern, reverse again, | ||
# and convert to row | ||
bit = bin(r)[2:].zfill(nq)[::-1] | ||
permuted_bit = "".join([bit[j] for j in qubit_pattern]) | ||
pr = int(permuted_bit[::-1], 2) | ||
if not invert: | ||
op_pat[pr] = r | ||
else: | ||
op_pat[r] = pr | ||
return op_pat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be done with numpy.transpose
in order to support the more general n-dimensional case. It looks something vaguely like this:
import functools
import numpy as np
a, b, c = np.random.rand(4, 4), np.random.rand(3, 3), np.random.rand(2, 2)
_012 = functools.reduce(np.kron, (a, b, c))
_120 = functools.reduce(np.kron, (b, c, a))
_210 = functools.reduce(np.kron, (c, b, a))
def permute(matrix, shape_r, order):
orig_shape = matrix.shape
split_shape = tuple(shape_r) * 2
new_order = tuple(order) + tuple(len(order) + x for x in order)
return matrix.reshape(split_shape).transpose(*new_order).reshape(orig_shape)
[
np.allclose(permute(_012, (4, 3, 2), (2, 1, 0)), _210),
np.allclose(permute(_012, (4, 3, 2), (1, 2, 0)), _120),
]
# [True, True]
(the arrays aren't bit-for-bit identical because I think the internal transpose
is done by abstract tensor-contraction, so involves a few FLOPs per matrix entry as opposed to doing the Kronecker product directly)
It'd want to test that for sure - I just did that super roughly. Mine is a double-sided permutation - you'd just tweak the left/right side of new_order
to be range(len(shape_r))
/range(len(shape_r), 2*len(shape_r))
to do a back/front-only permutation, I think, but I didn't totally test.
Mine currently assumes that the operator is square, but I think if you're only accepting one-sided permutations, you don't even strictly need that assumption. The shape
variables I've used here can probably be read out of Operator._op_shape
in some form - I don't remember the details off the top of my head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fwiw I didn't bother to check the endianness conventions when I wrote that - it was just a proof of principle. I might have done it backwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ikkoham and @jakelishman! I am trying to make @jakelishman's solution work (also many thanks to @gadial, who has a very similar conceptual solution), but all this business with endianness, interpreting permutation order, etc. is really confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reimplemented the code based on @jakelishman's and @gadial's code snippets. Took me a while to get all the indexing right. The new function can handle general qudits and one-sided permutations.
|
||
from qiskit.circuit.library import Permutation, PermutationGate | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In tests, we should be able to just import everything at the top of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Also, in terms of efficiencies - if my method is right, it likely wouldn't benefit much (if at all) from being moved to Rust, since all the work would already be happening in compiled Numpy code, and their |
Co-authored-by: Gadi Aleksandrowicz <[email protected]>
I have done a very quick experiment, and it seems that the implementation based on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sticking with this Sasha!
|
||
if not front: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty minor, but since this is an if/else
block, it's marginally clearer to reason about if the condition is "positive" (i.e. if front
/ else
). I get that not front
is the default, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed that, thanks
# shape: inv-permuted on left, original on right | ||
shape_l = self._op_shape.dims_l() | ||
shape_l = shape_l[::-1] | ||
shape_l = tuple(shape_l[x] for x in inv_perm) | ||
shape_l = shape_l[::-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got to say that this feels wrong to me - we seem to be splitting the matrix into a tensor with dimensions that don't match the actual input. Could you explain why this should be the case? Intuitively to me, it feels like the only thing that should vary based on the permutation is the rearrangement of the axes in the middle (the argument to transpose
).
Assuming you're right, I think a code comment explaining the algorithm would be very helpful here.
Just as a minor legibility note - lines 230 to 233 might be clearer as:
raw_shape_l = self._op_shape.dims_l()
n_dims_l = len(raw_shape_l)
shape_l = tuple(raw_shape_l[n_dims_l - n - 1] for n in reversed(inv_perm))
rather than reversing the tuple twice. The whole thing still feels mathematically odd to me, as described above, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have applied these and the following suggestions, making the code simpler and more readable, thanks!
inv_perm = np.argsort(perm) | ||
|
||
# shape: inv-permuted on left, original on right | ||
shape_l = self._op_shape.dims_l() | ||
shape_l = shape_l[::-1] | ||
shape_l = tuple(shape_l[x] for x in inv_perm) | ||
shape_l = shape_l[::-1] | ||
shape_r = self._op_shape.dims_r() | ||
split_shape = shape_l + shape_r | ||
|
||
# axes: permuted on left, id on right | ||
axes_l = tuple((np.argsort(inv_perm[::-1]))[::-1]) | ||
axes_r = tuple( | ||
self._op_shape._num_qargs_l + x for x in range(self._op_shape._num_qargs_r) | ||
) | ||
split_axes = axes_l + axes_r | ||
|
||
new_mat = ( | ||
self._data.reshape(split_shape).transpose(split_axes).reshape(self._op_shape.shape) | ||
) | ||
|
||
# updating shape: permuted on left, original on right | ||
new_shape_l = self._op_shape.dims_l() | ||
new_shape_l = new_shape_l[::-1] | ||
new_shape_l = tuple(new_shape_l[x] for x in perm) | ||
new_shape_l = new_shape_l[::-1] | ||
new_shape_r = self._op_shape.dims_r() | ||
|
||
new_op = Operator(new_mat, input_dims=new_shape_r, output_dims=new_shape_l) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to tell which parts of this code are logically shared between the two branches and which parts are different, so it's a bit tricky to read. I think you could re-organise this to
inv_perm = np.argsort(perm)
if front:
if len(inv_perm) != len(...):
...
shape_l = ...
shape_r = ...
axes_l = ...
axes_r = ...
new_shape_l = ...
new_shape_r = ...
else:
if len(inv_perm) != len(...):
...
shape_l = ...
...
split_shape = shape_l + shape_r
axes_order = axes_l + axes_r
new_matrix = self._data.reshape(split_shape).transpile(axes_order).reshape(self._op_shape.shape)
return Operator(new_matrix, input_dims=new_shape_r, output_dims=new_shape_l)
To me at least, this makes the logic clearer (assuming I'm correct) - the difference between front
and not front
is more obviously localised just to the construction of the shape / permutation axes, and the actual re-arrangement and construction logic is the same from then on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
for dims in [ | ||
(2, 3, 4, 5), | ||
(5, 2, 4, 3), | ||
(3, 5, 2, 4), | ||
(5, 3, 4, 2), | ||
(4, 5, 2, 3), | ||
(4, 3, 2, 5), | ||
]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to do this loop via ddt
parametrisation instead, so we can see individual test runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
# These are ok | ||
op.apply_permutation([2, 1, 0], front=False) | ||
op.apply_permutation([1, 0], front=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need to test these here, since the test is about the exceptions. We could test their full validity in a separate test, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
def test_reverse_qargs_as_apply_permutation(self): | ||
"""Test reversing qargs by pre- and post-composing with reversal | ||
permutation. | ||
""" | ||
perm = [3, 2, 1, 0] | ||
|
||
for dims in [ | ||
(2, 3, 4, 5), | ||
(5, 2, 4, 3), | ||
(3, 5, 2, 4), | ||
(5, 3, 4, 2), | ||
(4, 5, 2, 3), | ||
(4, 3, 2, 5), | ||
]: | ||
op = Operator( | ||
np.array(range(120 * 120)).reshape((120, 120)), input_dims=dims, output_dims=dims | ||
) | ||
op2 = op.reverse_qargs() | ||
op3 = op.apply_permutation(perm, front=True).apply_permutation(perm, front=False) | ||
self.assertEqual(op2, op3) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need an explicit test that applying only a one-sided permutation to an Operator
with heterogeneous tensored qudit spaces applies correctly (although I've got to say, I don't have a huge amount of intuition for what a clear test of that looks like).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also added tests for applying the permutation [1, 0] to operator with dimensions (2, 3) x (2, 3), where things can be easily worked out explicitly.
For completeness, here is Gadi's construction + proof of correctness (though please note that his notations are slightly different from Qiskit's).
What's important is that for math to work out, the shape has to be also permuted (something that I have also independently found out after countless experiments). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all this work, Sasha and Gadi. I'm going to approve this now - I'm happy that the maths we're using works out, though I don't think we're coding it in an intuitive way right now. The proof in this PR doesn't account for the Kronecker-product structure of the permutation that we're actually applying, while the code form we're doing is inherently related to that via transpose
. I'm content that the maths is correct for a pure row/col permutation, but it's currently unsatisfying to me about why we have to permute the tensor shape before we split it right now.
That said, I didn't have time to run my own maths on this to find a form I found more pleasant, and the tests seem to indicate that this is correct, so I don't want to hold up a correct implementation over me not fully clicking with the maths.
* adding apply_permutation method to Operator class * modify from_circuit to use apply_permutation * declaration fix * reimplementing based on review suggestions, but missing inversion * fixing inversion * fixing front vs back * adding test for reverse_qargs * Fixing operator dimensions and adding tests * Fixing operator dimensions and adding tests Co-authored-by: Gadi Aleksandrowicz <[email protected]> * pylint * adding tests for one-sided permutations on heterogenous qudit spaces * improving tests following review * using ddt * applying suggestions from review * applying suggestions from review --------- Co-authored-by: Gadi Aleksandrowicz <[email protected]>
Summary
Adds a method
apply_permutation
toOperator
class that modifies operator's data by composing it with a permutation. The permutation can be composed either before or after the operator This is achieved by directly swapping the rows or the columns of the array, and is more efficient than the more generalcompose
.Closes #9402.
Details and comments
This still misses a few tests and release notes. I am also not sure what is the most general version of the
Operator
, i.e. maybe this code should only work for unitary matrices.