-
Notifications
You must be signed in to change notification settings - Fork 353
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
MPS: Performance improvement when measuring all qubits #1277
Conversation
Also should review the heuristics that chooses between algorithms for measurement - the parameters should probably be changed. |
@yaelbh , @chriseclectic - please review |
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.
Very very nice
src/simulators/matrix_product_state/matrix_product_state_internal.cpp
Outdated
Show resolved
Hide resolved
src/simulators/matrix_product_state/matrix_product_state_internal.cpp
Outdated
Show resolved
Hide resolved
left_qubit = min_qubit == 0 ? 0 : min_qubit - 1; | ||
} | ||
if (num_qubits_ == 1) | ||
right_qubit = 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.
Same comment as in apply_measure_internal
. There is an unclear separation in the order of qubits according to apply_meausre
, which is perhaps not necessary.
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.
In the case of measure_all
, since we are measuring in order from 0
to n-1
, this will not really be necessary - there will be no propagation to the left. But I think it is better to cover this, for the sake of completeness. Let me know if you think I should do this differently.
@@ -1698,11 +1715,11 @@ void MPS::measure_reset_update_internal(const reg_t &qubits, | |||
mps_container_t MPS::copy_to_mps_container() { | |||
move_all_qubits_to_sorted_ordering(); | |||
mps_container_t ret; | |||
for (auto i=0; i<num_qubits(); i++) { | |||
for (uint_t i=0; i<num_qubits(); 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.
Why not auto
? By the way, when you used uint
before, perhaps it should be uint_t
?
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 changed all of these, because the compiler was giving warnings on them and I wanted to eliminate the warnings.
src/simulators/matrix_product_state/matrix_product_state_internal.hpp
Outdated
Show resolved
Hide resolved
src/simulators/matrix_product_state/matrix_product_state_internal.hpp
Outdated
Show resolved
Hide resolved
… algorithm for measure_all depends on the qubit ordering
…neighbors_internal
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.
Looks good. I'm not sure if doc string for "mps_sample_measure_algorithm" needs updating for this change or if it is an improvement to the existing options.
releasenotes/notes/mps-performance-of-measure-9991ac98da35150b.yaml
Outdated
Show resolved
Hide resolved
# --------------------------------------------------------------------- | ||
# Test MPS algorithms for measure | ||
# --------------------------------------------------------------------- | ||
def test_mps_measure_alg_qv(self): |
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.
It might be better to add these tests to the new AerSimulator tests which are a lot cleaner for specifying that they run on specific simulation methods or with custom simulation options.
Eg see https://github.com/Qiskit/qiskit-aer/blob/main/test/terra/backends/aer_simulator/test_algorithms.py#L68-L73 for example of running test on specific method with custom simulator options.
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 am not sure where would be most suitable for this test. Perhaps under https://github.com/Qiskit/qiskit-aer/blob/main/test/terra/backends/aer_simulator/test_options.py? on the other hand, it is testing measurement, so that's why I put it in test_measure
.
It's your call where to put 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.
Your last comment somehow disappeared. You mentioned putting the option apply_measure
in AerSimulator
and in QasmSimulator
. This option is not new. It is already in both these files.
@chriseclectic , am I correct to understand the following?
|
Hi @omarshehab, we didn't add another option |
@merav-aharoni , I understand that the default is |
@omarshehab , it may still be used, but that just depends on whether or not the heuristic chooses the |
Thanks, @mishmash for adding the clarification. The intent is indeed to implement this algorithm also when measuring a subset of the qubits, and to either remove or improve the heuristic accordingly. I am not sure I will get to this in the near future, so perhaps it is best to update the documentation, as you suggest. |
Summary
Performance improvement for the case where all qubits are measured.
Details and comments
In the previous implementation, after every qubit is measured, its effect is propagated to all other qubits. After that, the next qubit is measured. In the new implementation, if we are measuring all qubits, then it is not necessary to propagate to effect to all the remaining qubits. We only propagate to the nearest neighbors, because then we measure these neighbors next.
It is possible to implement for cases where a subset of the qubits is measured, but this will require additional effort to decide when propagation is necessary and when not.