-
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
Set different seed for each sampling in AerStatevector #1663
Conversation
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.
Is it always going to be the case that a user will want the RNG to advance by calling sample_memory
? I'm thinking probably yes, but I'm just mentioning it as a question.
self._aer_state = AerState(**self._aer_state.configuration()) | ||
|
||
configs = self._aer_state.configuration() | ||
if 'seed_simulator' in configs: | ||
configs['seed_simulator'] = int(configs['seed_simulator']) + 1 | ||
self._aer_state = AerState(**configs) |
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 seems like a slightly odd split in responsibilities. Perhaps it would be more convenient for now, and for future development if AerState
gains an advance_rng
method or something like that mutates the AerState
in place. It'll be easier to re-use between the different Aer quantum_info
classes you're planning that way, it gives clearer hints as to what's happening, and modifying it is kind of an AerState
internal feature that we don't particularly want client classes to need to know about.
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.
QuantumState
advances RNG. So, AerStatevector
also needs to advance it in sample_memory()
.
https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/quantum_info/states/quantum_state.py#L269
However, instead of manually update of seed_simulator
by increasing 1
, it is better to succeed the state of mt19937
from the previous AerState
.
src/controllers/state_controller.hpp
Outdated
int seed_; | ||
int seed_ = std::random_device()(); |
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.
What was happening before this change?
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.
Depending on environemnt. In my macbook (clang), it was always set 0
.
@jakelishman With 4b80871, |
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.
One comment aside, this looks like a much cleaner split of responsibilities to me, thanks!
@@ -661,13 +658,21 @@ void AerState::initialize() { | |||
|
|||
state_->initialize_qreg(num_of_qubits_); | |||
state_->initialize_creg(num_of_qubits_, num_of_qubits_); | |||
rng_.set_seed(seed_); |
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.
Do we still need to track the seed_
member after this change? I see it's still used in clear_ops()
, but it's not 100% clear to me why that function should reset the RNG. Admittedly, I'm not exactly clear on the circumstances in which it's called.
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.
qiskit-aer/src/controllers/state_controller.hpp
Line 1228 in b5b0c4d
last_result_.seed = buffer_.seed; |
This seed_
is to be returned as a member of a result. AerState.last_result()
returns it. We may not need to construct a result as AerSimulator
does for each call of methods, but it is not expensive to keep seed_
, I 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.
Ok, sounds good to me then!
* update seed for multiple calls of sample_memory * correct seed_simulator update in AerStatevector * set seed in AerState explicitly and reuse AerState in AerStatevector for sampling Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Summary
Previously seed is not initialized in AerStatevector and then sampled results are always same. With this commit, a seed is initialized for each sampling and sampled results can be vary.
Details and comments
Previously, sampled counts are always same in multiple calls because
AerStatevector
uses the same seed to generate random values which are used in processingsample_counts()
. More specifically,_seed
is not initialized instate_controller
ifseed_simulator
is not set. For example, following two sampled valuescount0
andcount1
are the same.Even if
seed_simulator
is set, the value is reused to initializeRngEngine
and sampled counts become same.With this PR,
_seed
is initialized withstd::random_device()
and renewedseed_simulator
for each sampling, and then sampled counts are always different in the sameAerStatevector
instance.