Fix measurement#290
Conversation
|
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. |
| "expval", | ||
| "MeasureAll", | ||
| "MeasureMultipleTimes", | ||
| "MeasureMultiPauliSum", |
There was a problem hiding this comment.
Why are we no longer adding this as a function to export?
There was a problem hiding this comment.
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
torchquantum/torchquantum/algorithm/hamiltonian.py
Lines 93 to 112 in 611cc2a
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:
torchquantum/torchquantum/measurement/measurements.py
Lines 270 to 271 in 611cc2a
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.
|
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 Edit: thanks for your comment just saw that the test is running correctly! |
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
means that we measure the expectation value of
X x Y x Z x Itensor product and then ofX 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, thusexpval_joint_analyticalis needed here. I have removed the prod fromMeasureMultiQubitPauliSum.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
expvalas implemented here. IMO it should be renamed toexpval_per_wireand 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_listtoMeasureMultipleTimesbutMeasureMultipleTimesdoes not take into account thecoefficiententry that is specified inMeasureMultiPauliSum. Thus,MeasureMultiPauliSumwill 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 useMeasureMultiQubitPauliSuminstead (and also removed the empty example in this directory).