-
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
Add Simulation method based on Stabilizer Rank Decompositions #51
Conversation
TODO: Add flag to enable NormEstimation TODO: Add UserGuide for the CH Simulator TODO: Pauli/Clifford noise support
TODO: Update tests, heuristic for switching to ch when using the qasm simulator in 'automatic' mode.
Defined for the CH simulator only; could consider making this a virtual method of the base controller class?
Thanks for the contribution @padraic-padraic , this is great! Remember to add some tests to your TODO list ;) |
Thanks for handling the review! The tests have been added and updated in my most recent commit; one caveat with this simulation method at the is its performance on strongly peaked output distributions, which is why the last build failed on the Grover case. Bumping up the number of shots leads to a reproducable pass on my end, which I'll push once I've added the QasmController integration. |
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 is great @padraic-padraic, I haven't looked through the code in detail but my main comment from a functional point of view would be to remove the standalone backend class and have it accessed through the QasmSimulator
by setting the simulation method.
I guess we would need to do some profiling to find a good automatic criterion, but I would say you want to check 2 conditions:
- The noise model is only Clifford + reset operations (like for the stabiliser method), so we don't accidentally add too many expensive t-gates into the circuit through noise sampling.
- The second condition I am imagining is a function of the number of t/u1 gates and qubit number, but I'm sure you or Sergey have a better ball-park idea for what this could be.
qiskit/providers/aer/aerprovider.py
Outdated
@@ -26,7 +27,8 @@ def __init__(self, *args, **kwargs): | |||
# Populate the list of Aer simulator providers. | |||
self._backends = [QasmSimulator(provider=self), | |||
StatevectorSimulator(provider=self), | |||
UnitarySimulator(provider=self)] | |||
UnitarySimulator(provider=self), | |||
CHSimulator(provider=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.
@padraic-padraic are you ok with removing the seperate backend and only having it accessed through QasmSimulator
using backend_options = {"method": "ch"}
?
We want to keep the number of backends in the provider down and have them based on their functional output, so all the backends that are designed to mimic a circuit experiment will come under the QasmSimulator
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.
Yeah, I think that would work. Most of the CHController is a shadow of the QasmController API anyway.
The one addition I made to the API is the required memory validation. We could add something like this to the QasmController? Or else I port it to Python, but either way we'd need to update the _validate method of QasmSimulator to account for the different footprints of the CH/Statevector methods.
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.
Adding the memory validation to the QasmController / Base Controller sounds good. We've been meaning to do something like this for awhile, since we should really be doing it for the statevector simulator as well (stabilizer too though in that case its memory footprint is comparatively tiny).
Currently the memory is checked in python before sending the qobj so it can fail-fast, but if we handle it correctly in the C++ we could remove that part from the python backend (or just do a very basic check there).
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.
For the moment I'll leave the simple check in Python, and set it to auto-switch to ch in the event that the number of qubits is too much for the statevector method (as long as it's not too high for the ch method). I guess the thing to do would be to make use of the required_memory_mb
method of state as part of an initialisation step in QasmController?
One question I have is for calling This also has relevance for when we auto-switch to |
@padraic-padraic yes we may need to rethink that on the Terra side, but for now the default basis gates includes the most general basis gates for the simulator (ie the statevector method). This means by default if it has gates like The general gateset shouldn't be a problem for the automatic method as it will check if gates in the circuit are supported on the different methods and choose one. If a user tried to force running on the stabilizer or CH simulator on a circuit with |
Just a note on this; I've got most of the benchmarks done to implement a heuristic swticher for CH/QASM, and I've added memory validation in the QasmController as part of validate_state (which takes into account experiment parallelisation. To avoid cross-platofrm headaches, I used However, I've been running into some odd intermittent crashes when calling the CH simulator from within Python. I'm just working on debugging those, hopefully should have a sense of what's going on by tomorrow! |
Hi both, This is pretty much done from my end! Here's what's in the latest 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.
Great Job @padraic-padraic,
I haven't finished the review, but I left some comments regarding coding style.
I'll follow the review later.
@@ -114,6 +115,9 @@ def _format_qobj_str(self, qobj, backend_options, noise_model): | |||
if backend_options is not None: | |||
for key, val in backend_options.items(): | |||
config[key] = val | |||
if not "available_memory" in config: |
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.
As we may want to offer the possibility to the user to specify the available_memory
through a config parameter, I'd rather calculate the real available memory in the simulator side if the user doesn't set this parameter.
@@ -152,6 +185,9 @@ def _validate(self, qobj, backend_options, noise_model): | |||
'result data will not contain counts.', name) | |||
# Check if Clifford circuit or if measure opts missing | |||
no_measure = True | |||
ch_supported = False |
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 about:
ch_supported = True if method == "ch" or method == "automatic" else False
... using the same structure than the following expression.
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 went for
ch_supported = method in ['ch', 'automatic']
to keep the line short!
self.name(), system_memory) | ||
if method != "automatic": | ||
raise AerError(err_string + '.') | ||
else: |
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.
There's no need for this else
here... raising will terminate the function.
We can decrease the indentation then
raise AerError(err_string + ', and has too many ' + | ||
'qubits to fall back to the ' + | ||
'CH simulator.') | ||
elif not ch_supported: |
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.
No need for elif
, a simple if
will be enough as the above raise
will terminate the function
raise Aererror(err_string + ', and contains ' + | ||
'instructions not supported by ' + | ||
'the CH simulator.') | ||
else: |
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 we can remove this else
as well for the same reasons exposed before
src/simulators/ch/ch_runner.hpp
Outdated
const U1Sample t_sample(t_angle); | ||
const U1Sample tdg_sample(tdg_angle); | ||
|
||
uint_t zero = 0ULL; |
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.
A constant, so:
const uint_t ZERO = 0LL;
src/simulators/ch/ch_runner.hpp
Outdated
uint_t three_qubit_gate_count; //Needed for normalising states | ||
std::vector<chstabilizer_t> states; | ||
std::vector<complex_t> coefficients; | ||
uint_t n_omp_threads; |
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.
num_threads
is a more descriptive name for this variable.
src/simulators/ch/ch_runner.hpp
Outdated
|
||
std::unordered_map<double, U1Sample> z_rotations; | ||
|
||
void InitMetropolis(AER::RngEngine &rng); |
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.
We always use lower_snake_case
for naming functions and variables, and UpperCamelCase
for classes.
so this should be:
void init_metropolis( ... )
src/simulators/ch/ch_runner.hpp
Outdated
std::unordered_map<double, U1Sample> z_rotations; | ||
|
||
void InitMetropolis(AER::RngEngine &rng); | ||
void MetropolisStep(AER::RngEngine &rng); |
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 as above:
void metropolis_step()
src/simulators/ch/ch_runner.hpp
Outdated
bool check_omp_threshold(); | ||
|
||
//Convert each state to a json object and return it. | ||
std::vector<std::string> serialised_decomposition() const; |
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.
functions performs actions, so a better name for this functions could be:
std::vector<std::string> serialize_decomposition() const;
src/simulators/ch/ch_runner.hpp
Outdated
//Measure a Pauli projector on each term in the decomposition and update their coefficients | ||
// omega. | ||
void apply_pauli_projector(const std::vector<pauli_t> &generators); | ||
inline void apply_pauli_projector(const std::vector<pauli_t> &generators, uint_t rank); |
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.
There's no need for specifying inline
in header files
src/simulators/ch/ch_runner.hpp
Outdated
inline void apply_pauli_projector(const std::vector<pauli_t> &generators, uint_t rank); | ||
//Routine for Norm Estimation, thin wrapper for the CHSimulator method that uses AER::RngEngine | ||
//to set up the estimation routine. | ||
double NormEstimation(uint_t n_samples, AER::RngEngine &rng); |
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.
NormEstimation
-> norm_estimation
src/simulators/ch/ch_runner.hpp
Outdated
//Routine for Norm Estimation, thin wrapper for the CHSimulator method that uses AER::RngEngine | ||
//to set up the estimation routine. | ||
double NormEstimation(uint_t n_samples, AER::RngEngine &rng); | ||
double NormEstimation(uint_t n_samples, std::vector<pauli_t> generators, AER::RngEngine &rng); |
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.
NormEstimation
-> norm_estimation
src/simulators/ch/ch_runner.hpp
Outdated
double NormEstimation(uint_t n_samples, std::vector<pauli_t> generators, AER::RngEngine &rng); | ||
|
||
//Metropolis Estimation for sampling from the output distribution | ||
uint_t MetropolisEstimation(uint_t n_steps, AER::RngEngine &rng); |
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.
MetropolisEstimation
-> metropolis_estimation
src/simulators/ch/ch_runner.hpp
Outdated
|
||
//Metropolis Estimation for sampling from the output distribution | ||
uint_t MetropolisEstimation(uint_t n_steps, AER::RngEngine &rng); | ||
std::vector<uint_t> MetropolisEstimation(uint_t n_steps, uint_t n_shots, AER::RngEngine &rng); |
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.
MetropolisEstimation
-> metropolis_estimation
src/simulators/ch/ch_runner.hpp
Outdated
uint_t MetropolisEstimation(uint_t n_steps, AER::RngEngine &rng); | ||
std::vector<uint_t> MetropolisEstimation(uint_t n_steps, uint_t n_shots, AER::RngEngine &rng); | ||
//Efficient Sampler for the output distribution of a stabilizer state | ||
uint_t StabilizerSampler(AER::RngEngine &rng); |
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.
StabilizerSampler
-> stabilizer_sampler
src/simulators/ch/ch_runner.hpp
Outdated
std::vector<uint_t> MetropolisEstimation(uint_t n_steps, uint_t n_shots, AER::RngEngine &rng); | ||
//Efficient Sampler for the output distribution of a stabilizer state | ||
uint_t StabilizerSampler(AER::RngEngine &rng); | ||
std::vector<uint_t> StabilizerSampler(uint_t n_shots, AER::RngEngine &rng); |
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.
StabilizerSampler
-> stabilizer_sampler
uint_t StabilizerSampler(AER::RngEngine &rng); | ||
std::vector<uint_t> StabilizerSampler(uint_t n_shots, AER::RngEngine &rng); | ||
//Utilities for the state-vector snapshot. | ||
complex_t amplitude(uint_t x_measure); |
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 can be const
:
complex_t amplitude(uint_t x_measure) const;
src/simulators/ch/ch_runner.hpp
Outdated
|
||
void Runner::initialize(uint_t num_qubits) | ||
{ | ||
if (states.size() > 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.
I think we can safely remove the conditionals and just call:
states.clear();
coefficients.clear();
src/simulators/ch/ch_runner.hpp
Outdated
|
||
void Runner::initialize_omp(uint_t n_threads, uint_t threshold_rank) | ||
{ | ||
if(n_threads == 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.
There's a ternary operator in C++ that is very handy for this simple situations:
n_omp_threads = (n_threads == 0 ? 1 : n_threads);
src/simulators/ch/ch_runner.hpp
Outdated
|
||
void Runner::initialize_decomposition(uint_t n_states) | ||
{ | ||
chi = n_states; |
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.
We don't need to create a chi
variable, do we? Just use n_states
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.
chi
(which I'll rename chi_
) is the member variable. I used n_states
as the argument to avoid name duplication, which will be fixed by the underscore convention.
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'll probably switch from chi_
(which is lifted from the paper) to n_states_
for clarity.
coefficients.reserve(chi); | ||
if(states.size() > 1 || coefficients.size() > 1) | ||
{ | ||
throw std::runtime_error(std::string("CHSimulator::Runner was initialized without") + |
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.
There's no need to surround literal text strings with std::string()
. You can just use sentences surrounded by double quotes, like:
throw std::runtime_error("CHSimulator::Runner was initialized without"
" being properly cleared since the last "
" experiment.");
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.
Ahh okay; I was using + operators to join the component strings and getting compiler errors. That makes sense.
src/simulators/ch/ch_runner.hpp
Outdated
complex_t coeff(coefficients[0]); | ||
for(uint_t i=1; i<chi; i++) | ||
{ | ||
states.push_back(base_sate); |
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.
hmmm is this right? Do we want to fill states vector with the same base_state
instance?
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.
We do yeah. This simulator only branches at non-Clifford gates. If the circuit starts with some m
Clifford gates, then we basically run a clifford simulator for those m
gates, and then setup a decomposition by taking chi
copies of this initial state and doing the usual sampling method. This saves us having to repeateadly apply these first m
gates to all the states in the decomposition.
src/simulators/ch/ch_runner.hpp
Outdated
std::vector<uint_t> adiag_1(n_samples, 0ULL); | ||
std::vector<uint_t> adiag_2(n_samples, 0ULL); | ||
std::vector< std::vector<uint_t> > a(n_samples, std::vector<uint_t>(n_qubits, 0ULL)); | ||
#pragma omp parallel for if(n_omp_threads > 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.
I think we can use OMP collapse(3)
clause here:
#pragma omp for collapse(3) if(n_omp_threads > 1)
src/simulators/ch/ch_runner.hpp
Outdated
class Runner | ||
{ | ||
private: | ||
uint_t n_qubits; |
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.
Please, suffix all class variables with an ending underscore:
uint_t n_qubits_;
uint_t chi_;
...
src/simulators/ch/ch_runner.hpp
Outdated
{ | ||
accept = 0; | ||
//Random initial x_string from RngEngine | ||
uint_t max = (1ULL<<n_qubits) - 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.
I'm not a big fun of this type we have created: uint_t
which is really a 64 bits unsigned integer... not your fault though, we have been using it all over the code base.
using chstate_t = CHSimulator::Runner; | ||
using Gates = CHSimulator::Gates; | ||
|
||
uint_t zero = 0ULL; |
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.
const uint_t ZERO = 0ULL;
using Gates = CHSimulator::Gates; | ||
|
||
uint_t zero = 0ULL; | ||
uint_t toff_branch_max = 7ULL; |
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.
const uint_t TOFF_BRANCH_MAX = 7ULL;
src/simulators/ch/ch_state.hpp
Outdated
//Allowed error in the stabilizer rank decomposition. | ||
//The required number of states scales as \delta^{-2} | ||
//for allowed error \delta | ||
double approximation_error = 0.05; |
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.
Private class variables should be suffixed with underscore: approximation_error_ = 0.05
src/simulators/ch/ch_state.hpp
Outdated
//for allowed error \delta | ||
double approximation_error = 0.05; | ||
|
||
uint_t norm_estimation_samples = 100; |
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.
Sames as above: norm_estimation_samples_
src/simulators/ch/ch_state.hpp
Outdated
// How long the metropolis algorithm runs before | ||
// we consider it to be well mixed and sample form the | ||
// output distribution | ||
uint_t metropolis_mixing_steps = 7000; |
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.
metropolis_mixing_steps_
src/simulators/ch/ch_state.hpp
Outdated
uint_t metropolis_mixing_steps = 7000; | ||
|
||
//Minimum number of states before we try to parallelise | ||
uint_t omp_threshold_rank = 100; |
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.
omp_threshold_rank_
src/simulators/ch/ch_state.hpp
Outdated
//Minimum number of states before we try to parallelise | ||
uint_t omp_threshold_rank = 100; | ||
|
||
double snapshot_chop_threshold = 1e-10; |
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.
snapshot_chop_threshold_
src/simulators/ch/ch_state.hpp
Outdated
inline void compute_extent(const Operations::Op &op, double &xi); | ||
|
||
//Compute the required chi, and count the number of three qubit gates | ||
virtual std::pair<uint_t, uint_t> decomposition_parameters(const std::vector<Operations::Op> &ops); |
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'd remove virtual
keyword here, if we are not planning to subclass this class.
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 this can be const
as well
src/simulators/ch/ch_state.hpp
Outdated
std::pair<bool, size_t> check_stabilizer_opt(const std::vector<Operations::Op> &ops); | ||
|
||
//Check if we can use the sample_measure optimisation | ||
bool check_measurement_opt(const std::vector<Operations::Op> &ops); |
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.
const
src/simulators/ch/ch_state.hpp
Outdated
virtual std::pair<uint_t, uint_t> decomposition_parameters(const std::vector<Operations::Op> &ops); | ||
|
||
//Check if this is a stabilizer circuit, for the locaiton of the first non-CLifford gate | ||
std::pair<bool, size_t> check_stabilizer_opt(const std::vector<Operations::Op> &ops); |
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.
const
src/simulators/ch/ch_state.hpp
Outdated
std::pair<uint_t, uint_t> State::decomposition_parameters(const std::vector<Operations::Op> &ops) | ||
{ | ||
double xi=1.; | ||
uint_t three_qubit_gate_count = 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.
Do we need a 64 bit integer to have this counter? I'd promote it to: u32_t
.
|
||
std::pair<bool, size_t> State::check_stabilizer_opt(const std::vector<Operations::Op> &ops) | ||
{ | ||
for(auto op = ops.cbegin(); op != ops.cend(); op++) |
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 is a good candidate for the new for( : )
you used before:
for(auto& op : ops){
...
}
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 left this as is because make use of pointer arithmetic later in the function (returning op - op.cbegin()
)
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 looks good to go @padraic-padraic! If you've finished with @atilag 's comments I'm happy to merge once the conflict with master is resolved. Was there anything else left you wanted to do in this PR?
Hey Chris, I’ve resolved that conflict now! Some kind of odd white space issue is all. I’m happy for this to be merged now. In a future update I’d like to add u2/u3/U decompositions but for now this is useable as is. |
) * Add CHSimulator, backend wrapper, standalone executable * Add json for standalone tests * Tests for CH Simulator, optimised measurement for a single stabilizer state * Make sure we use optimised measurement everywhere * Measurement Tests * CHSimulator passes all default_basis tests TODO: Add flag to enable NormEstimation TODO: Add UserGuide for the CH Simulator TODO: Pauli/Clifford noise support * Switch to direct Toffoli Decomposition * Update controller, state and python wrappers for new validation API TODO: Update tests, heuristic for switching to ch when using the qasm simulator in 'automatic' mode. * Add method for checking memory requirements Defined for the CH simulator only; could consider making this a virtual method of the base controller class? * Tests fixed for 0.1.1 * QasmController::run_circuit_helper for ch method * Style changes to tests * Memory validation, Automatic Switching to CH * Add documentation
Summary
Details and comments
Resolves #47.
This is a work in progress to add a simulator for circuits with a small number of non-Clifford gates, but with support for up to 63 qubits.
Currently, it exists as a standalone c++ simulator and controller. To integrate it with the QASMController we still need
method == 'automatic'
.qiskit.providers.aer.QasmSimulator.validate
andqiskit.providers.aer.CHSimulator.validate
.