-
Notifications
You must be signed in to change notification settings - Fork 358
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
Add subsystem density matrix snapshots to qasm simulator #718
Add subsystem density matrix snapshots to qasm simulator #718
Conversation
1f0da85
to
60b093f
Compare
60b093f
to
e78a334
Compare
8eb578a
to
d189a27
Compare
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.
First round.
Will finish later!
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.
Besides some suspicious using of std::move
, there's something about the usage of qreg_.vector()
that would be great to rethink in order to avoid a huge data copy.
data.add_average_snapshot("density_matrix", | ||
op.string_params[0], | ||
BaseState::creg_.memory_hex(), | ||
std::move(reduced_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.
I would pass reduced_state
as a reference (const?) instead of moving it. Both are really references, but the move semantics only adds noise here.
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.
The data container has special functions for move semantics, so passing the std move here is needed to use that. Otherwise reduced_state will be copied (and it is potentially a very large matrix).
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.
My point is that we don't need move semantics here because there's no need for transfer ownership and using the overloaded add_average_snapshot
method that passes reduced_state
as a const reference would avoid the copy and will be even faster than casting to rvalue.
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.
Almost there!! :)
data.add_average_snapshot("density_matrix", | ||
op.string_params[0], | ||
BaseState::creg_.memory_hex(), | ||
std::move(reduced_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.
My point is that we don't need move semantics here because there's no need for transfer ownership and using the overloaded add_average_snapshot
method that passes reduced_state
as a const reference would avoid the copy and will be even faster than casting to rvalue.
template <> | ||
cmatrix_t State<QV::QubitVectorThrust<float>>::density_matrix(const reg_t &qubits) { | ||
return vec2density(qubits, BaseState::qreg_.vector()); | ||
} | ||
|
||
template <> | ||
cmatrix_t State<QV::QubitVectorThrust<double>>::density_matrix(const reg_t &qubits) { | ||
return vec2density(qubits, BaseState::qreg_.vector()); | ||
} |
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.
template <typename floating_t>
cmatrix_t State<QV::QubitVectorThrust<floating_t>>::density_matrix(const reg_t &qubits) {
return vec2density(qubits, BaseState::qreg_.vector());
}
We can do this because both specializations have the same implementation, so the compiler will derive using monomorphization
(specialize double
and float
if they are used in the code at compile time).
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 tried this change but it resulted in compiler errors like:
error: nested name specifier 'State<QV::DensityMatrixThrust<floating_t> >::' for declaration does not refer into a class, class template or class template partial specialization
cmatrix_t State<QV::DensityMatrixThrust<floating_t>>::reduced_density_matrix(...
for (size_t k = 0; k < END; k++) { | ||
// store entries touched by U | ||
const auto inds = QV::indexes(qubits, qubits_sorted, k); | ||
for (size_t row = 0; row < DIM; ++row) |
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.
And these too
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.
This one is a reduction so for one you would have to define a custom reduction type for OMP here, and I think that would result in memory for the matrix being duplicated across threads. For these types of snapshots which can be very large in memory usage I think minimizing memory is more important than speed.
Further testing showed these do avoid a copy
Summary
Blocked by #712Details and comments