negativity_cost_fn and density qnode functions #53
Conversation
There was a problem hiding this comment.
Hey @m-bhatia, these changes look great and will make a fine addition to qnetvo! I've left a few comments detailing some changes to the existing code. In addition to these changes:
- Please add unit tests for each of the functions you add (
density_matrix_qnode,negativity_cost_fn, andpartial_transpose). The tests are located in thetestdirectory and the naming and format should be pretty straightforward. - We use
blackto autoformat code. To pass the styling checks, please runblack -l 100 src testto reformat your changes to conform to the black styling guidelines.
Let me know if you have any questions, thanks again for your contribution!
P.S. The qnetvo contributing guidelines might be helpful.
src/qnetvo/cost/negativity.py
Outdated
| bra_indices[-i], ket_indices[-i] = ket_indices[-i], bra_indices[-i] | ||
|
|
||
| einsum_str = ''.join(indices) + '->' + ''.join(bra_indices + ket_indices) | ||
| result = np.einsum(einsum_str, dm_reshaped) |
There was a problem hiding this comment.
let's use qml.math.einsum here if it works
src/qnetvo/cost/negativity.py
Outdated
|
|
||
| density_qnode = reduced_density_matrix_qnode(network_ansatz, wires, **qnode_kwargs) | ||
|
|
||
| def partial_transpose(dm, m, n): |
There was a problem hiding this comment.
Two things:
- the
partial_transposefunction should have a docstring. - we should move this function out of
negativity_cost_fnbecause it is very basic and could be used broadly. At this point, theutilities.pyis probably the best place for it to go.
src/qnetvo/cost/negativity.py
Outdated
| dims = [2] * (2 * (m + n)) | ||
| dm_reshaped = dm.reshape(dims) | ||
|
|
||
| indices = list('abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ')[:2 * (m + n)] |
There was a problem hiding this comment.
Let's throw a ValueError for the case when m + n > 52, the code won't behave as expected in this case.
There was a problem hiding this comment.
I changed the partial_transpose function so that it no longer has this limitation
| :rtype: Function | ||
| """ | ||
|
|
||
| if len(wires) != m + n: |
src/qnetvo/cost/negativity.py
Outdated
| def negativity_cost_fn(network_ansatz, m, n, wires, qnode_kwargs={}): | ||
| """Constructs an ansatz-specific negativity cost function. | ||
|
|
||
| Negativity can be used to identify if two subsystems :math:`A` and :math:`B` are entangled, through the PPT criterion. Negativity is an upper bound for distillable entanglement. |
There was a problem hiding this comment.
If there is an arxiv paper for this or other good online resource, please add a link
| :returns: A cost function ``negativity_cost(*network_settings)`` parameterized by | ||
| the ansatz-specific scenario settings. | ||
| :rtype: Function | ||
| """ |
There was a problem hiding this comment.
Please add a :raises ValueError: in the docstring to document the constraints on m and n
As an example, you can see docstring for the qnetvo.NetworkAnsatz class.
src/qnetvo/qnodes.py
Outdated
| @qml.qnode(qml.device(**network_ansatz.dev_kwargs), **qnode_kwargs) | ||
| def circuit(settings): | ||
| network_ansatz.fn(settings) | ||
| return qml.density_matrix(wires=network_ansatz.network_wires) |
There was a problem hiding this comment.
| return qml.density_matrix(wires=network_ansatz.network_wires) | |
| return qml.density_matrix(wires=network_ansatz.layers_wires[-1]) |
In general, there may be ancillary wires in the network that aren't measured. To be more consistent with other qnodes use network_ansatz.layers_wires[-1]. The current convention in qnetvo is for all measurement nodes to be placed in the final layer, so this will get the density matrix on all measurement wires.
src/qnetvo/qnodes.py
Outdated
| def reduced_density_matrix_qnode(network_ansatz, wires, **qnode_kwargs): | ||
| """Constructs a qnode that computes the density matrix in the computational basis | ||
| across all specified wires. | ||
|
|
||
| :param network_ansatz: A ``NetworkAnsatz`` class specifying the quantum network simulation. | ||
| :type network_ansatz: NetworkAnsatz | ||
|
|
||
| :param wires: The wires on which the node operates. | ||
| :type wires: list[int] | ||
|
|
||
| :returns: A qnode called as ``qnode(settings)`` for evaluating the reduced density matrix of the | ||
| network ansatz. | ||
| :rtype: ``pennylane.QNode`` | ||
| """ | ||
|
|
||
| @qml.qnode(qml.device(**network_ansatz.dev_kwargs), **qnode_kwargs) | ||
| def circuit(settings): | ||
| network_ansatz.fn(settings) | ||
| return qml.density_matrix(wires) | ||
|
|
||
| return circuit |
There was a problem hiding this comment.
We should just wrap this into density_matrix_qnode because it is the same functionality. To do this, we'll add a wires kwarg as density_matrix_qnode(network_ansatz, wires=None, **qnode_kwargs). Then, we add the condition wires = network_ansatz.layers_wires[-1] if wires == None else wires
… functions with test cases updated functionality of partial transpose so it works with any input size
|
I made the requested changes, as well as edited the partial_transpose function so it can work for larger density matrices. The qNetVO contributing guidelines were very helpful. Thank you! |
There was a problem hiding this comment.
Great Changes @m-bhatia , Thanks for getting this done.
I made a few addittions to fix some formatting issues with the docs, but everything else looked super solid. I'll merge once the tests pass. This functionality will be available in qnetvo. v0.4.5
No description provided.