-
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
Add partial transpose function in quantum_info #9566
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:
|
Pull Request Test Coverage Report for Build 4706198971
💛 - Coveralls |
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 your contribution! I left some general comments below 🙂
qiskit/quantum_info/states/utils.py
Outdated
raise QiskitError("Indices of subsystems to be transposed are invalid") | ||
ptden = np.empty((2**n, 2**n), complex) | ||
ptden[:] = np.nan | ||
if isinstance(state, Statevector): |
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 don't quite understand what a partial transpose of a vector is, do you have a definition for this? 🙂 Maybe this operation should only be supported for actual DensityMatrix
objects, as you also mentioned in the typehint?
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.
Hey @Cryoris, it seems what you're saying is true. We had considered allowing input as a Statevector and converting it to a pure state density matrix, similar to partial trace code. However, if you're suggesting that it's not necessary, we can remove that option. Please let us know.
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 a user would like to do that they could always construct the DensityMatrix explicitly and then use your function:
dm = DensityMatrix(state)
partial_transpose(dm, ...)
so I think you don't have to cover this case here 🙂
qiskit/quantum_info/states/utils.py
Outdated
for i in range(2**n): | ||
for j in range(2**n): | ||
if math.isnan(ptden[i, j]): |
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.
Could this loop be made more efficient by only visiting entries that we know need to be populated, or maybe transposing sub-blocks of the matrix all at once? As it is now, this scales as 2 ^ (2n)
which is going to be very expensive.
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 will try to optimize it.
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.
For most operators, I would imagine you can do this in vectorised Numpy by casting state.data
(a Numpy array) to a multi-dimensional tensor (e.g. using np.reshape(state.data, state._op_shape.tensor_shape)
or something like that), calling np.transpose
as appropriate, then restoring it to be a 2D array. If that's correct, then that should be much faster than looping in Python space.
quantum_info.Operator
also isn't guaranteed to be square. I don't think the partial trace is reasonably defined for tensor-product spaces where the number of input and output spaces are unequal, but it could in theory work for things like a mapping of a (2, 3)
Hilbert space to a (1, 4)
one. Probably best not to worry about that, though, and just error out if the dims_r
of the OpShape
aren't equal to the dims_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.
@jakelishman This helps. Thank you.
qiskit/quantum_info/states/utils.py
Outdated
""" | ||
state = _format_state(state, validate=False) | ||
n = len(state.dims()) | ||
lst = np.zeros(2**n, int) |
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.
The DensityMatrix
also allows qudit subsystems (see the dims
argument 🙂), but in this implementation you assume qubits. We'd either have to add a check that the input density matrix is defined on qubits or generalize this code to work on arbitrary dimensions.
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 for bringing this to our attention. We will take steps to rectify the issue.
@ikkoham requested changes have been incorporated, and we kindly request your assistance for further process. |
@ikkoham, @Cryoris, and @jakelishman, could any one of you please tell us what is the progress regarding the pull request? |
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. Sorry for late. This direction looks good to me.
inplace
option can be optional and feature request in the future.
I left some comments.
""" | ||
from qiskit.quantum_info.states import utils | ||
|
||
state = utils._format_state(self, validate=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.
Why do you need _format_state
and reshape
? self
is DensityMatrix
by definition.
rho1[4, 5] = 0.5 | ||
rho1[5, 4] = 0.5 | ||
rho1[5, 5] = 0.5 | ||
self.assertEqual(DensityMatrix.partial_transpose(rho, [0, 1]), DensityMatrix(rho1)) |
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.
DensityMatrix.partial_transpose
is not classmethod
, so DensityMatrix(rho).partial_transpose([0, 1])
is correct.
@PayalSolanki2906 @pranay1990 Thank you for your great contributions. I would like to merge this PR for 0.24. Please check my PR to your repo. PayalSolanki2906#1 |
Thank you for allowing us to make our first contribution. We sincerely appreciate the support and guidance provided during the process, which made it very smooth for us. We are eager to continue contributing in the future and look forward to working with you again. |
Hi @ikkoham, after this pull request is merged, we would be interested in contributing towards the implementation of logarithmic negativity as an entanglement measure. We would appreciate your input on whether this is a feasible addition and would be grateful for any guidance or feedback you may have. Thank you for your time and consideration. |
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! LGTM.
For the log negativity, I think a function would be nice (not method), but let's discuss. I would be glad if you could make an issue,
* partial_transpose included * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included test for partial transpose * included test for partial transpose * included test for partial transpose * added docstring * included DensityMatrix(rho1) * changed rho1 * addition of release note * fix * fix * fir partial_transpose * Update utils.py * refactor and add tests --------- Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
* partial_transpose included * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included partial_transpose * included test for partial transpose * included test for partial transpose * included test for partial transpose * added docstring * included DensityMatrix(rho1) * changed rho1 * addition of release note * fix * fix * fir partial_transpose * Update utils.py * refactor and add tests --------- Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Hello @ikkoham, @PayalSolanki2906, @pranay1990. I just bump into this new This can be easily fixed by just having the function return return DensityMatrix(rho, dims=self.dims()) Not sure if a separate PR needs to be issued since this one has already been merged, but happy to help if needed. |
Diego: if there's a bug, we'll need a PR to fix it. Could you open an issue with an example showing the bug, then you (or us, if you don't have time) can make a PR fixing it. |
Summary
The partial transpose of a density matrix is a mathematical operation that transforms the matrix by interchanging only certain indices of the matrix elements. This operation is significant in the study of quantum entanglement as it provides information about the entanglement structure of a quantum system, with positive partial transpose indicating separable states and negative partial transpose indicating entangled states.
Details and comments
I and @pranay1990 have contributed to this code. We have added a new function named partial_transpose in quantum_info (under utility functions).