-
Notifications
You must be signed in to change notification settings - Fork 603
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
Qcut sampling postprocessing function #2358
Conversation
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
…ithub.com:PennyLaneAI/pennylane into qcut-sample-subgraphs-to-fragment-tape-conversion
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
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.
Thanks @anthayes92!
tests/transforms/test_qcut.py
Outdated
for pp, exp_pp in zip(postprocessed, expected_postprocessed): | ||
assert np.allclose(pp, exp_pp) |
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 pp, exp_pp in zip(postprocessed, expected_postprocessed): | |
assert np.allclose(pp, exp_pp) | |
assert np.allclose(postprocessed[0], expected_postprocessed[0]) |
Do we need the loop if there's only one element?
tests/transforms/test_qcut.py
Outdated
gives the correct results. | ||
""" | ||
|
||
def test_sample_postprocess(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.
Might be good to have an interface-based test like here, where we also check that the returned is converted to the right interface type.
tests/transforms/test_qcut.py
Outdated
for pp, exp_pp in zip(postprocessed, expected_postprocessed): | ||
assert np.allclose(pp, exp_pp) | ||
|
||
def test_mc_sample_postprocess(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.
Similarly for this test, would be good to check for all the interfaces.
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.
These tests now iterate over all interfaces.
tests/transforms/test_qcut.py
Outdated
fixed_samples, communication_graph, fixed_settings, shots, func | ||
) | ||
|
||
expected = 85.33333333333333 |
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 be tempted to add some spy functionality (e.g., to classical_processing_fn
,prod
, and potentially hstack
) to check the provided inputs are as expected.
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
tests/transforms/test_qcut.py
Outdated
|
||
fixed_settings = np.array([[0, 7, 1], [5, 7, 2], [1, 0, 3], [5, 1, 1]]) | ||
|
||
# spy_func = mocker.spy(fn) |
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 sure how to spy on this function, generally we use mocker.spy(package, "function")
but this function exists by itself so I only have a single arg to pass to spy
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, can leave out for now 👍
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.
Thanks @anthayes92! Just left a few more comments - would it also be possible to fix what is mentioned here?
tests/transforms/test_qcut.py
Outdated
] | ||
|
||
postprocessed = qcut.qcut_processing_fn_sample(fixed_samples, communication_graph, shots) | ||
postprocessed = qml.math.cast_like(postprocessed, lib.ones(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.
Why are we casting the output to the interface type? We need to cast the input to the interface type and check that the output is of the same type.
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 for the test below)
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.
ah I got mixed up! changed here: fd328ac
tests/transforms/test_qcut.py
Outdated
|
||
fixed_settings = np.array([[0, 7, 1], [5, 7, 2], [1, 0, 3], [5, 1, 1]]) | ||
|
||
# spy_func = mocker.spy(fn) |
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, can leave out for now 👍
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Thanks for the suggestions @trbromley , I've updated the docsting for |
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.
Thanks @anthayes92. I've left a final round of comments but should be good to merge once resolved.
tests/transforms/test_qcut.py
Outdated
np.array([[0.0], [-1.0], [-1.0]]), | ||
np.array([[1.0], [1.0], [1.0]]), | ||
] | ||
cast_fixed_samples = [qml.math.cast_like(fs, lib.ones(1)) for fs in fixed_samples] |
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.
cast_fixed_samples = [qml.math.cast_like(fs, lib.ones(1)) for fs in fixed_samples] | |
cast_fixed_samples = [qml.math.convert_like(fs, lib.ones(1)) for fs in fixed_samples] |
This is an important distinction.
tests/transforms/test_qcut.py
Outdated
fragment_configurations, settings = qml.transforms.qcut.expand_fragment_tapes_mc( | ||
fragment_tapes, communication_graph, shots | ||
) |
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.
fragment_configurations, settings = qml.transforms.qcut.expand_fragment_tapes_mc( | |
fragment_tapes, communication_graph, shots | |
) |
tests/transforms/test_qcut.py
Outdated
np.array([[0.0], [-1.0], [-1.0]]), | ||
np.array([[1.0], [1.0], [1.0]]), | ||
] | ||
cast_fixed_samples = [qml.math.cast_like(fs, lib.ones(1)) for fs in fixed_samples] |
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.
cast_fixed_samples = [qml.math.cast_like(fs, lib.ones(1)) for fs in fixed_samples] | |
cast_fixed_samples = [qml.math.convert_like(fs, lib.ones(1)) for fs in fixed_samples] |
tests/transforms/test_qcut.py
Outdated
postprocessed = qcut.qcut_processing_fn_mc( | ||
fixed_samples, communication_graph, fixed_settings, shots, fn | ||
) | ||
postprocessed = qml.math.cast_like(postprocessed, lib.ones(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.
postprocessed = qml.math.cast_like(postprocessed, lib.ones(1)) |
I don't think we need this - we should be testing that postprocessed
is the expected type without casting.
tests/transforms/test_qcut.py
Outdated
spy_hstack = mocker.spy(np, "hstack") | ||
|
||
postprocessed = qcut.qcut_processing_fn_mc( | ||
fixed_samples, communication_graph, fixed_settings, shots, fn |
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.
fixed_samples, communication_graph, fixed_settings, shots, fn | |
cast_fixed_samples, communication_graph, fixed_settings, shots, fn |
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Thanks for the suggestions @trbromley . Updates to the tests have been manually checked and added here: ceb1383 |
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
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.
Thanks @anthayes92!
Context:
The pipeline for cutting a circuit with sampling measurements requires it own postprocessing capabilities. This will support returning bitstrings as well as expectation values, the choice of which should be specified by the user.
Description of the Change:
Add sampling postprocessing function
Benefits:
Facilitates cutting circuits with sampling measurements
Possible Drawbacks:
Potential computational bottleneck depending on number of fragments circuits to be processed