-
Notifications
You must be signed in to change notification settings - Fork 368
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
Improve performance of average snapshots #807
Conversation
Previously the mean and variance were returned as a copy of stored accumulated data. This allows them to modify the stored data and return a reference that can be moved into result containers which can help avoid copies for large data objects like probability and density matrix snapshots. To make this work it was also necessary to allow JSON conversion of results to be non const.
return (count_ > 1) ? Linalg::div(T(accum_), double(count_)) : accum_; | ||
void AverageData<T>::normalize() { | ||
if (!normalized_ && count_ >= 1) { | ||
if (count_ > 1) { |
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.
If we move the mean
and variance
results, subsequent calls to those functions will yield wrong results. Probably we don't need to call them again but if that's the case we may need to implement something to recompute the values, as normalize
won't be executed again as normalized_
will be true
. What do 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.
It should be fine to call mean and variance multiple times, only the first call will result in normalizing the data (this is because normalize
does nothing if normalized_ = true
). But if you move them then yes, you've destroyed the container. I think this is like any other C++ object, after you used move you shouldn't use it again. If you want to use it again you should copy or reference the data rather than move, however i don't think this situation comes up in our current use case.
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 the movement is a bit more hidden because we are moving specific members instead of the whole object, but as we're not gonna reuse them it seems we are ok.
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.
Lgtm! Nice performance improvement!
Summary
Previously the mean and variance were returned as a copy of stored
accumulated data. This allows them to modify the stored data and return
a reference that can be moved into result containers which can help
avoid copies for large data objects like probability and density matrix
snapshots. To make this work it was also necessary to allow JSON conversion of
results to be non const.
Details and comments
Memory profiler comparisons for following circuit:
For this PR: time_py = 29.2 s, time_cpp = 26.7 s
For master: time_py = 31.8 s, time_cpp = 58.3 s
And memory profiling PR:
vs Master: