-
Notifications
You must be signed in to change notification settings - Fork 61
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
Expectation values from samples #675
Conversation
for more information, see https://pre-commit.ci
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 implementing this, it's a useful feature. I wrote two comments below, let me know what you think.
for j, k in enumerate(keys): | ||
index = 0 | ||
for i in qubit_map: | ||
index += int(k[qubit_map.index(i)]) * 2 ** (kl - 1 - i) |
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.
Here you can avoid the conversion from binary to decimal if freq
is given in decimal. If these frequencies are result of some circuit execution, the user can do result.frequencies(binary=False)
to get this without much effort.
Even if you get the frequencies in binary, I believe you can still avoid the for loops if the operation is vectorized. For example something like
ids = tuple(map(lambda x: int(x, 2), freq.keys())) # covert indices to decimal
O = np.diag(Obs)[ids] * counts
I would also find a better name for the variable O
because it does not look very nice in the code. I generally avoid single letter names and capitals in local variables. O
is even more confusing because it's very close to 0
.
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 the suggestion. The user can include a qubit_map
that indicates which qubit each position in the counts refers to. For example, let's consider a 6 qubit circuit where we only measure qubits 1, 3 and 5 because we want to get an expected value of an observable that only acts on those qubits. We will then get counts for the states 000, 001, 010 ...111. When converting these numbers to decimal we have to take into account the qubit_map
because the first position indicates qubit 1, the second position indicates qubit 3 and the third position indicates qubit 5. With this in mind, I do not see a clear way to do it in the way you propose. If you have any suggestion, let me know and I will implement it.
I agree that the variable names are not appropriate so I have made some changes. Let me know what you think.
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 the explanation, you are right. I think some part of this calculation can still be vectorized, even with the qubit_map
, but I am not sure if you gain anything.
If you expect freq
to be a result of circuit execution, in principle you could instead take the the CircuitResult
object, which is what circuit execution returns and contains both the frequencies and the qubit_map. This could be simpler in some cases as the user would not need to specify the qubit map twice, it would be read directly from the measurement gate in the circuit. However, what you are doing here is more flexible because it also works when frequencies do not come from a circuit.
A way to have both approaches is to define an additional method CircuitResult.expectation_from_samples(hamiltonian)
which calls the method you defined here. So one can do
result = c(nshots=100)
result.expectation_from_samples(hamiltonian)
I am not sure if this is useful, let me know what you think.
I agree that the variable names are not appropriate so I have made some changes. Let me know what you think
Thanks, looks better now.
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #675 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 84 84
Lines 10886 11015 +129
==========================================
+ Hits 10886 11015 +129
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@stavros11 please let me know if you agree with the new changes so we can merge? |
src/qibo/tests/test_hamiltonians.py
Outdated
state = c() | ||
freq = result.frequencies(binary=True) | ||
Obs0 = hamiltonians.Hamiltonian(3, matrix).expectation_from_samples( | ||
freq, qubit_map=None | ||
) | ||
Obs1 = h1.expectation(c().state()) |
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.
Here the state
variable is not used anywhere.
Also you don't need to execute the circuit twice to get the state, you can just do result.state()
directly.
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 spotting this. Corrected.
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 adding an aditional method to CircuitResult
is useful so I have implemented it.
for j, k in enumerate(keys): | ||
index = 0 | ||
for i in qubit_map: | ||
index += int(k[qubit_map.index(i)]) * 2 ** (kl - 1 - i) |
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 the explanation, you are right. I think some part of this calculation can still be vectorized, even with the qubit_map
, but I am not sure if you gain anything.
If you expect freq
to be a result of circuit execution, in principle you could instead take the the CircuitResult
object, which is what circuit execution returns and contains both the frequencies and the qubit_map. This could be simpler in some cases as the user would not need to specify the qubit map twice, it would be read directly from the measurement gate in the circuit. However, what you are doing here is more flexible because it also works when frequencies do not come from a circuit.
A way to have both approaches is to define an additional method CircuitResult.expectation_from_samples(hamiltonian)
which calls the method you defined here. So one can do
result = c(nshots=100)
result.expectation_from_samples(hamiltonian)
I am not sure if this is useful, let me know what you think.
I agree that the variable names are not appropriate so I have made some changes. Let me know what you think
Thanks, looks better now.
for more information, see https://pre-commit.ci
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 the updates. If you are not planning to add anything else, I believe this is ready.
In this PR I implemented the computation of expectation values of
Zs
operators from counts. The input is the frequenciesresult.frequencies(binary=True)
and a mapping for the qubits indices. For example, suppose we have a 10 qubit circuit but we are measuring 5 of them[1,3,5,7,9]
and we want to compute an observable that acts on those or less qubitsZ(5)*Z(3)*Z(9)
. Then,qubit_map=[1,3,5,7,9]
.Checklist: