Skip to content
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

Fix measurement #290

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

yannick-couzinie
Copy link

@yannick-couzinie yannick-couzinie commented Nov 20, 2024

This is sort of a follow-up on #288, I misunderstood the functions, the fix there was wrong (as was the original function).

If I understand correctly, the example in the docstring

class MeasureMultipleTimes(tq.QuantumModule):
    """
    obs list:
    list of dict: example
    [{'wires': [0, 2, 3, 1], 'observables': ['x', 'y', 'z', 'i']
    },
    {'wires': [0, 2, 3, 1], 'observables': ['x', 'y', 'z', 'i']
    },
    ]
    """

means that we measure the expectation value of X x Y x Z x I tensor product and then of X x Y x Z x I (i.e. the same state).

We expect this to return [batch_size, len(obs_list)] and not [batch_size, len(obs_list), num_wires], you cannot get anything separated by wires from here, the expectation value of the tensor product is just a real number, thus expval_joint_analytical is needed here. I have removed the prod from MeasureMultiQubitPauliSum.

I have added a test in test/measurements/ to make sure that the analytical result is recovered, please check for yourself.

In fact, I don't really see any use case for expval as implemented here. IMO it should be renamed to expval_per_wire and it should be made explicit that the observables are looped over and the i-th return value is the expectation value of the i-th observable w.r.t. to the i-th wire (with the rest traced out). What is the use case for this over just expval_joint_analytical?

Further, I have removed MeasureMultiPauliSum, it passes obs_list to MeasureMultipleTimes but MeasureMultipleTimes does not take into account the coefficient entry that is specified in MeasureMultiPauliSum. Thus, MeasureMultiPauliSum will not fail (since it is a correct dict with too many keys) but also not return the correct value. Since I have no interest in implementing this correctly, I would remove. You can check that the coefficient entry has no effect by changing the coefficients in the pauli_sum_op.py example file. I have changed this file to use MeasureMultiQubitPauliSum instead (and also removed the empty example in this directory).

@01110011011101010110010001101111
Copy link
Collaborator

01110011011101010110010001101111 commented Nov 20, 2024

Thanks for the correction, and I appreciate adding the tests! I’m not very familiar with the function myself so I figured that if the tests passed, it would be fine, though our tests are certainly not complete. I’ll give a more stylistic/code review shortly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this file be removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an empty file in the examples section, I do not really see what function it is accomplishing, so I removed it, but I can put it back if you do not feel comfortable removing it.

@@ -23,7 +23,6 @@
"expval",
"MeasureAll",
"MeasureMultipleTimes",
"MeasureMultiPauliSum",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we no longer adding this as a function to export?

Copy link
Author

@yannick-couzinie yannick-couzinie Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the function, because it was not correctly implemented. You can check the example pauli_sum_op.py, changing the coefficients has no effect on the outcome of the measurement (see my first post). The coefficient part of the input is not taken into account.

Thinking about it, I think we could implement this easily by using

class Hamiltonian(object):
"""Hamiltonian class."""
def __init__(self,
coeffs: List[float],
paulis: List[str],
endianness: str = "big",
) -> None:
"""Initialize the Hamiltonian.
Args:
coeffs: The coefficients of the Hamiltonian.
paulis: The operators of the Hamiltonian, described in strings.
endianness: The endianness of the operators. Default is big. Qubit 0 is the most significant bit.
Example:
.. code-block:: python
coeffs = [1.0, 1.0]
paulis = ["ZZ", "ZX"]
hamil = tq.Hamiltonian(coeffs, paulis)
"""

and using the matrix of the hamiltonian.

Alternatively, we could change expval_joint_analytical to not only take the string of observables, but also coefficient info and multiply the coefficients at the point where the kronecker product is taken here:

for op in observable[1:]:
hamiltonian = torch.kron(hamiltonian, pauli_dict[op].to(states.device))

But then we would have to change the other expval functions as well to match this. If you think there is a real use case for this measurement and it is worth changing the expval functions we can do that, otherwise just remove as done here.

@yannick-couzinie
Copy link
Author

yannick-couzinie commented Nov 20, 2024

Yes it also seems the tests are not run in the github actions, only the examples folder, it might be worth fixing that by adding the tests to test.sh or better make it run as a unit-testing suite. I'll add an issue for that.

Edit: thanks for your comment just saw that the test is running correctly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants