-
Notifications
You must be signed in to change notification settings - Fork 585
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 unit tests for simulate_tree_mcm
#6231
base: master
Are you sure you want to change the base?
Conversation
Hello. You may have forgotten to update the changelog!
|
simulate_tree_mcm
simulate_tree_mcm
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6231 +/- ##
==========================================
+ Coverage 99.58% 99.71% +0.12%
==========================================
Files 444 443 -1
Lines 42367 42020 -347
==========================================
- Hits 42193 41899 -294
+ Misses 174 121 -53 ☔ View full report in Codecov by Sentry. |
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.
Looks good. Many of the tests don't verify the numerical correctness of the tests. For that, we would probably need a strategy similar to #6124, which is blocked by discussions.
Seconding Mudit here, can we verify the numerical correctness where it makes sense? |
@mudit2812 and @astralcai I have added numerical assertions to |
tests/devices/qubit/test_simulate.py
Outdated
@pytest.mark.unit | ||
@pytest.mark.parametrize("shots", [None, 5500]) | ||
@pytest.mark.parametrize("postselect", [None, 0]) | ||
@pytest.mark.parametrize("reset", [False, True]) | ||
@pytest.mark.parametrize("measure_f", [qml.counts, qml.expval, qml.probs, qml.sample, qml.var]) | ||
@pytest.mark.parametrize( | ||
"meas_obj", [qml.Y(0), [1], [1, 0], "mcm", "composite_mcm", "mcm_list"] | ||
) | ||
def test_simple_dynamic_circuit(self, shots, measure_f, postselect, reset, meas_obj): |
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.
Instead of marking the individual tests as unit
, you can mark the whole test 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.
LGTM
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.
Just a few comments to resolve before I can approve.
tests/devices/qubit/test_simulate.py
Outdated
@pytest.mark.unit | ||
def test_tree_traversal_deep_circuit(self): | ||
"""Test that `simulate_tree_mcm` works with circuits with many mid-circuit measurements""" | ||
|
||
n_circs = 500 | ||
operations = [] | ||
for _ in range(n_circs): | ||
operations.extend( | ||
[ | ||
qml.RX(1.234, 0), | ||
(m0 := qml.measure(0, postselect=1)).measurements[0], | ||
qml.CNOT([0, 1]), | ||
qml.ops.op_math.Conditional(m0, qml.RZ(1.786, 1)), | ||
] | ||
) | ||
|
||
qscript = qml.tape.QuantumScript( | ||
operations, | ||
[qml.sample(wires=[0, 1]), qml.counts(wires=[0, 1])], | ||
shots=20, | ||
) | ||
|
||
mcms = find_post_processed_mcms(qscript) | ||
assert len(mcms) == n_circs | ||
|
||
split_circs = split_circuit_at_mcms(qscript) | ||
assert len(split_circs) == n_circs + 1 | ||
for circ in split_circs: | ||
assert not [o for o in circ.operations if isinstance(o, qml.measurements.MidMeasureMP)] | ||
|
||
res = simulate_tree_mcm(qscript, postselect_mode="fill-shots") | ||
assert len(res[0]) == 20 | ||
assert isinstance(res[1], dict) and sum(list(res[1].values())) == 20 |
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 don't really understand the purpose of this test. If we check the correctness of simulate_tree_mcm
with at least 3 MCMs, then I don't see the point in checking that the results are correct for 500 MCMs.
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 was meant to test that simulate_tree_mcm
can handle sufficiently large numbers of mcms
. I brought it here from default_qubit_native_mcm
and extended it to test the auxiliary functions the tree-traversal algorithm uses for finding mcms
that require post-processing and for splitting the tape on those particular mcms
.
for rs1, rs2 in zip(res1, res2): | ||
prob_dist1, prob_dist2 = rs1, rs2 | ||
if measure_f in (qml.sample,): | ||
n_wires = rs1.shape[1] | ||
prob_dist1, prob_dist2 = np.zeros(2**n_wires), np.zeros(2**n_wires) | ||
for prob, rs in zip([prob_dist1, prob_dist2], [rs1, rs2]): | ||
index, count = np.unique( | ||
np.packbits(rs, axis=1, bitorder="little").squeeze(), return_counts=True | ||
) | ||
prob[index] = count | ||
|
||
assert qml.math.allclose( | ||
sp.stats.entropy(prob_dist1 + 1e-12, prob_dist2 + 1e-12), 0.0, atol=5e-2 | ||
) |
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.
Just curious how you arrived at this testing strategy? @maliasadi would love your input here as well.
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.
The motivation here is to calculate the KL divergence (KLD) between the two probability distributions computed from the samples obtained from the two tapes. It gives a measure that shows how different are distributions.
Context: Adds unit tests for the functionality to simulate mid-circuit measurements natively.
Description of the Change:
Benefits:
Possible Drawbacks:
Related GitHub Issues: [sc-71875]