Add CPU simulators test suite with 100+ initial tests#2905
Add CPU simulators test suite with 100+ initial tests#2905orpuente-MS wants to merge 9 commits intomainfrom
Conversation
orpuente-MS
requested review from
billti,
idavis,
minestarks and
swernli
as code owners
| } | ||
| // The first vector is the zero vector. | ||
| // We return `true` iff the second vector is also the zero vector. | ||
| None => other.data.iter().all(|b| b.norm() <= TOLERANCE), |
There was a problem hiding this comment.
Do you normalize? If so, is this even possible to have all values so small?
| .data | ||
| .iter() | ||
| .zip(other.data.iter()) | ||
| .find(|(a, _)| a.norm() > TOLERANCE) |
There was a problem hiding this comment.
Consider finding the largest by norm. May improve precision.
| //! | CZ | CZ|x0⟩ = |x0⟩, CZ(a,b) = CZ(b,a) | | ||
| //! | SWAP | Exchanges states, SWAP^2 ~ I | | ||
| //! | MZ | MZ ~ MZ MZ (idempotent, does not reset) | | ||
| //! | RESET | OP RESET ~ I (resets to |0⟩) | |
There was a problem hiding this comment.
RESET and RESETZ aren't equivalent to I. They change the state of anything other than 0. However, check_programs_are_eq only check for starting in 0 state. This feels a bit misleading: on one hand, "A equivalent to B" sounds like the two should be the same operators (for example, XX ~ I). One the other hand, the code only checks if they are the same starting from 0 input. One needs to use something like Choi–Jamiołkowski isomorphism to test equivalence.
Having said that, I'm not sure you need to test the full equivalence for these basic tests. You probably need to clarify the definition of equivalence. Or, better yet, don't call it equivalence (even though it is an equivalence relation, just not the one people expect), and say something like "producing the same state starting from zero".
| //! | ||
| //! `T`, `T_ADJ`, `Rx`, `Ry`, `Rz`, `Rxx`, `Ryy`, `Rzz` (non-Clifford gates) | ||
| //! | ||
| //! # Gate Properties |
There was a problem hiding this comment.
Testing various equivalences starting from 0 state is a very useful addition to test!
One thing feels missing - more basic test. I think for primitive gates we should have tests that check them on all basis vectors. I.e. If we have a one-qubit gate, we check it on |0> and |1>. If we have two-qubit gate, we check it on |00>, |01>, |01>, |11>. If we had a way to set the state before the gate and check the state after the gate, these tests could be as simple as going over a table of each state for each gate.
I'm not sure if we can access or even keep state in different simulators. And this doesn't have to be part of this PR - something to think about when checking basic functionality.
There was a problem hiding this comment.
This is a really good idea. What if we add logic to check_programs_are_eq! to make it so that it runs tests starting from all possible basis vectors instead of just starting from zero? This is not perfect, but it's better than just checking from zero. For example, it would make the test checking that I is equivalent to MResetZ fail, which is good, since that test should be written in another way.
There was a problem hiding this comment.
It feels like existing way of writing tests would be still heavy for these basic tests. I think just something like a table should be more appropriate. Gate, Input state -> result state. That way it will be all on one page easy to grasp. And a function that will go over this table and execute checks.
| //! | ||
| //! # Gate Properties | ||
| //! | ||
| //! The `~` symbol denotes equivalence up to global phase. |
There was a problem hiding this comment.
Same note on equivalence as in the other file.
| } | ||
|
|
||
| #[test] | ||
| fn x_noise_on_h_gate_affects_superposition() { |
There was a problem hiding this comment.
X noise on H gate does affect the state but does not affect the outcome of this program. No matter what X noise you apply, the probability distribution should be 50/50. Test is fine, rename maybe?
| shots: 1000, | ||
| seed: SEED, | ||
| noise: noise_config! { | ||
| cz: { xi: 0.1 }, |
There was a problem hiding this comment.
Again, this noise doesn't affect outcome.
| shots: 1000, | ||
| seed: SEED, | ||
| noise: noise_config! { | ||
| x: { x: 0.2 }, |
There was a problem hiding this comment.
Again, noise doesn't affect outcome.
|
|
||
| #[test] | ||
| fn noise_accumulates_across_multiple_gates() { | ||
| // Two X gates, each with noise - errors compound |
There was a problem hiding this comment.
The interesting part here is to check the probabilities more precisely. The probability of 0 should be 0.9^2+0.1^2=0.82 and the probability of 1 should be 0.90.12=0.18. The only way to test this reliably is to run this test manually without a seed and with very large number of shots. We cannot really have automatic seedless tests with large number of shots so we can only check that results are reproducible with the seed and small number of shots.
I think this should have a comment on what to do if the test fails due to some change in the simulator. Something like - if updating expected value ensure that the probability of 0 is ... and 1 is ...
| shots: 1000, | ||
| seed: SEED, | ||
| noise: noise_config! { | ||
| h: { x: 0.02 }, |
There was a problem hiding this comment.
X noise on H doesn't affect this program.
| h: { x: 0.02 }, | ||
| cx: { xi: 0.02, ix: 0.02 }, | ||
| }, | ||
| format: top_n(4), |
There was a problem hiding this comment.
There could be only 4 outcomes, right?
| }, | ||
| format: top_n(4), | ||
| output: expect![[r#" | ||
| 00: 491 |
There was a problem hiding this comment.
00: 0.5 * 0.96
11: 0.5 * 0.96
01: 0.5 * 0.02 * 2
10: 0.5 * 0.02 * 2
| seed: SEED, | ||
| noise: noise_config! { | ||
| intrinsics: { | ||
| 0: { x: 0.2 }, |
There was a problem hiding this comment.
I suggest significantly different noise probability. Like 0.1 and 0.5.
| let within_rev: Vec<QirInstruction> = { | ||
| let mut v = qir!($($within_tt)*); // expand tokens again for reverse | ||
| v.reverse(); | ||
| v |
There was a problem hiding this comment.
This just reverses the gate order? This looks awfully dangerous. The resulting language looks similar to Q# and one would expect inverse of gates. Even if now all the gates are self-inverse (I haven't checked) I suggest you don't call it "within-apply". Or add a hard explicit check that all gates here are self-inverse.
There was a problem hiding this comment.
Thanks for the feedback! I totally missed this detail. I can change this to verify that the instructions are indeed adjointable and use their adjoint. This should be simple to add.
| /// output: expect![[r#"..."#]], | ||
| /// } | ||
| /// ``` | ||
| macro_rules! check_sim { |
There was a problem hiding this comment.
Do you really need a macro for this? I'm a bit concerned that macro here is just and additional complexity. Could it be a function? What would be less convenient if it becomes a function?
There was a problem hiding this comment.
That is a first concern. In this case, the macro is doing two things:
- First, they buy us named, optional parameters. Rust doesn't have default arguments or named parameters for functions. If this were a function, every test would have multiple arguments set to
None, and it would be harder to tell what arguments are.
In tests that quickly hurts readability. For example:
check_sim<StabilizerSimulator>(
qir! {
x(0);
mresetz(0, 0);
},
1,
1,
1,
None,
None,
raw,
expect![[r#"1"#]],
)Compare that to the macro version:
check_sim! {
simulator: StabilizerSimulator,
program: qir! {
x(0);
mresetz(0, 0);
},
num_qubits: 1,
num_results: 1,
output: expect![[r#"1"#]],
}With the macro the test is shorter and easier to read / understand, specially in platforms like GitHub without lang-server support.
This matters because we will have hundreds of tests.
- Hiding syntax, semantics, and machinery that isn't the point of the test. I feel this will become more evident when I try to integrate the GPU simulator in the test-suite. The GPU simulator is quite different from the CPU ones. Trying to work around the type system to accomodate an API for the test suite that works for both could be possible (I am not sure yet), but if it is, it will require more work than just having a macro. And if we pulled it off, the test suite will likely end with syntax and semantics that are concerned with Rust's generic type system, instead of minimal syntax and semantics that are concerned with what is being tested.
| /// num_qubits: 1, | ||
| /// } | ||
| /// ``` | ||
| macro_rules! check_programs_are_eq { |
There was a problem hiding this comment.
Also this one. What improves when this is a macro?
There was a problem hiding this comment.
See my answer to the previous question.
2 participants
The main purpose of this PR is to build some infrastructure for writing tests for the CPU simulators. I also added tests for each gate of every simulator verifying known properties of those gates, but I might be missing something.
The easiest way to review this PR is to go to these four new files:
full_state_noiseless.rsfull_state_noisy.rsclifford_noiseless.rsclifford_noisy.rsand read the module level docstring at the top of the file. That should have a summary with all the properties being tested for each gate in that file. And if you are interested on how a property is being tested you can jump to the corresponding test.
Missing in this PR: tests for CY and CCX gates, since those are still unimplemented.