Execute Rust circuit using NumpyBackend
#26
Conversation
| @@ -102,14 +111,16 @@ def plus_density_matrix(self, nqubits): | |||
| def matrix(self, gate): | |||
| """Convert a gate to its matrix representation in the computational | |||
| basis.""" | |||
| name = gate.__class__.__name__ | |||
| name = type(gate).__name__.replace("Gate_", "") | |||
There was a problem hiding this comment.
I don't fully like this. Open to suggestions on how to make better (map gates to matrices: relevant for simulation backends).
There was a problem hiding this comment.
This is something I was thinking about (maybe I even already told you at some point): we might want to add the matrices in Rust (e.g. with ndarray) and feature gate them (i.e. optionally compile).
I was thinking that we could even accept a gate with a given number of qubits to be applied to more qubits, always treating the excess ones as controllers (internally, maintaining the |
In 90ed2ea I tried to replace |
|
@stavros11 should we consider this as superseding #25, and thus close it? |
Yes, I will close #25 since it was never intended to be merged. About this, I would say maybe you could review and merge if you agree. The primary goal, which is to execute the new minimal circuit is achieved, and there isn't really much more to do unless we start adding features to the Rust circuit and gates. However, we already have issues for this and will do in other PRs anyway. |
There was a problem hiding this comment.
I tried to run the tests and the example, and everything works as expected.
I'm leaving a few comments, and there are only two things I'd like to try before merging:
- returning a separate iterator
- get rid of the einsum dance
In any case, none of them is strictly needed, not strictly needed in this PR, so we can merge whenever we need this.
| @@ -1 +1,5 @@ | |||
| from . import numpy | |||
| from .numpy import NumpyBackend | |||
| from .qibo_core import * | |||
There was a problem hiding this comment.
It turns out that this is working properly.
Thus, I'd keep it with the full re-export, at least all the public elements of the compiled extension are exposed in the package, without the need of any further intervention.
Eventually, we might even drop the Python layer, when the backend mechanism will be implemented, and the NumpyBackend moved back to Qibo (or wherever else).
| if nqubits + len(qubits) > len(EINSUM_CHARS): # pragma: no cover | ||
| raise_error(NotImplementedError, "Not enough einsum characters.") |
There was a problem hiding this comment.
I will replace EINSUM_CHARS before merging, maybe it will be enough to get rid of the whole module, or most of it.
| @@ -468,7 +445,7 @@ def execute_circuits( | |||
| circuits, initial_states, nshots, processes, backend=self | |||
| ) | |||
|
|
|||
| def execute_circuit_repeated(self, circuit, nshots, initial_state=None): | |||
| def execute_circuit_repeated(self, circuit, nshots, initial_state=None, density_matrix=False): | |||
There was a problem hiding this comment.
I guess the reason behind this addition is only that .density_matrix is not any longer a property of the circuit, correct?
However, assuming that's the case, I'd even be tempted to keep it like this. Though, it could be useful as a property: a circuit containing channels should always be executing with a density matrix, and if there are no channels, the density matrix could always be constructed at the end. Isn't it?
The only other option is applying a circuit on a density matrix initial_state, but this you should be able to find out from the type of the initial state itself, so no need to store the information in the circuit once more.
There was a problem hiding this comment.
I.e. the execution should be on a density matrix if initial_state.density_matrix or circuit.has_channels.
There was a problem hiding this comment.
I forgot to respond to this yesterday. This particular line is temporary, as I haven't thought about repeated execution yet. As it is now it won't work with the qibo-core Circuit anyway, but it is also not relevant before we have channels or collapse measurements.
To answer the questions.
I guess the reason behind this addition is only that
.density_matrixis not any longer a property of the circuit, correct?
Yes, that is correct. The idea was to treat density_matrix as an execution option, instead of Circuit attribute, since the same circuit could potentially be executed both with state vectors and density matrices (more on that below).
However, assuming that's the case, I'd even be tempted to keep it like this. Though, it could be useful as a property: a circuit containing channels should always be executing with a density matrix, and if there are no channels, the density matrix could always be constructed at the end. Isn't it?
Mostly correct, except that some channels can be simulated with state vectors + repeated execution (this is basically what this function is for). In that case we don't get a state vector in the end, but we can still get shots. This requires sampling the channel, which is for sure possible for some of them (for example PauliNoiseChannel), but I am not sure if it is for all. Intermediate collapse measurements require similar handling to channels in that case.
I think the question is whether we want to give the user the flexibility of choosing whether to use density matrices or no. If yes, I would probably prefer to treat it as execution option. If no, circuit property would work, but then if all channels automatically fall back to density matrices, the repeated execution will no longer be a feature.
The only other option is applying a
circuiton a density matrixinitial_state, but this you should be able to find out from the type of the initial state itself, so no need to store the information in thecircuitonce more.
A potential issue with this is that we support different types of initial states:
None(default): gives no information about whether the user expects a density matrix or no,Circuit: since we don't store.density_matrixinCircuitanymore, also no information,- array/tensor supported by the backend: no direct information, but in principle we could infer if it's a vector or a matrix from the shape.
There was a problem hiding this comment.
I agree with or acknowledge what you wrote above.
A potential issue with this is that we support different types of initial states:
About this in particular:
None(default): gives as no information about whether the user expects a density matrix or no,Circuit: since we don't store.density_matrixthere anymore, also no information,
These two are equivalent: the second is just the first where the circuit passed is the concatenation of the two, I'm not even sure we should allow this at high-level (since there should be only one way of doing things, whenever possible).
Thus, we could either assume the most basic option (always state vector, to lower the memory need) or the most appropriate (state vector if no channels/collapse, else density matrix). I'd go for the most basic.
- array/tensor supported by the backend: no direct information, but in principle we could infer if it's a vector or a matrix from the shape.
That's exactly what I had in mind: infer from the shape.
In principle, we could wrap the array with a Qibo object, that will be aware of being itself a state vector or density matrix for some system.
Currently, it would just be overhead.
As soon as we will start handling the backends results with memory references, and split the state manipulation library from the execution backend itself, it would not be useless any longer. In that case, we won't even need to inspect the shape, but just the object.
| if len(circuit.measurements) == 0 and not circuit.has_collapse: | ||
| raise RuntimeError( | ||
| "Attempting to perform noisy simulation with `density_matrix=False` and no Measurement gate in the Circuit. If you wish to retrieve the statistics of the outcomes please include measurements in the circuit, otherwise set `density_matrix=True` to recover the final state." | ||
| ) |
There was a problem hiding this comment.
E.g. here, if we just avoided the density_matrix, we could instead set it here as:
density_matrix = len(circuit.measurements) == 0 and not circuit.has_collapseand avoid the error.
There was a problem hiding this comment.
Btw, this error would be raised even for density_matrix=True.
There was a problem hiding this comment.
Here I just moved it from execute_circuit to execute_circuit_repeated temporarily, without thinking much if it makes sense. I need to check more carefully all the different combinations of flags that lead to repeated execution, because there may be redundancies on these errors/checks. I will open an issue about repeated execution, to understand to what extend all these flags are relevant for qibo-core.
There was a problem hiding this comment.
Ok, as soon as you have the issue, feel free to close this comment.
|
Thanks for the review and comments.
As said above, I am not sure how much I can help with this.
For this, I probably can help. I just didn't do it since there is already an issue. I am not sure if you were already working on it, if not feel free to assign the issue to me. |
|
@stavros11 in c26ad9f I implemented the iteration over the Python object in the most idiomatic way I knew. Once the This could have been suboptimal, if we were making use of the state of the iterator in order to avoid repeating unrequired computation. But since the previous iterator was doing exactly the same, I just replaced it with the default Python one. However, this does not mean that is not possible to have a separate iterator object. Even more, that's exactly described in the second example here: |
|
Ok, all comments have been solved, or postponed to later PRs and issues (#33, #34). The only open one is the einsum dance, but this is more of a Qibo issue, since this code is only temporarily present in Let's merge :) |
First prototype and some examples/tests. Not heavily tested (we could use the Qibo tests for that, at some point), but it should be possible to execute circuits that contain only the gates that are currently in Rust.
There are several commented lines in
NumpyBackend.execute_circuit. These will be dropped or uncommented depending onCircuitdesign choices. There are several attributes of qiboCircuits and gates that do not exist in the Rust implementation yet (or most may not be ported at all). For now I noticed the following:gate.is_controlled_by: Not relevant anymore since the gate doesn't contain qubits. In order to maintain this functionality, we should probably add an optionalcontrols: Vec<usize>argument (default=empty) incircuit.add, to allow controlling gates on arbitrary number of qubits (we already have the simulation code for this).circuit.measurements: I added it as an empty vector for now. It will not be empty once we port the measurement gate.circuit.density_matrix: I am not convinced this is needed. It is more of an execution option (with some caveats)circuit.repeated_execution: Same as above.circuit.has_collapse: Same as above, it could also be inferred real time, but we need to first port measurements before discussing about this.circuit.accelerators: Almost certain we should remove, move to execution option, ie.backend.execute_circuit(..., accelerators=...)for backends that support this, which I believe is only qibojit-cupy (this is for multi-GPU only).circuit.has_unitary_channel: Not 100% sure what it is.I believe the next priorities are:
controlled_by(?) (should be easy to add)c.add(gates.X(0))(qubits in gates) interface in Qibo?