-
Notifications
You must be signed in to change notification settings - Fork 127
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 support for conditional tomography #1021
Add support for conditional tomography #1021
Conversation
b4bbc24
to
0269533
Compare
0269533
to
b07ee7b
Compare
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.
If I understand correctly updated tomography performs multiple subfit for each conditional fragment and thus this PR is adding new layer of conditional cregs to existing fitter modules and post analysis is also updated to format multiple outcome data for each fragment. It is bit tough to understand implementation (especially numerical part) details only from the code, so this review is mainly for machinery part. I feel conditional is bit confusing and I would use different term, but it's okey if this name is acknowledged by the community.
I believe utility and fitter functions work as you expect (because there is only integration test), however, it would be better to introduce some dedicated data structure for fragmented outcome for better code readability. It was indeed tough to infer the value from the variable name and type hint because they were just ndarray
.
d360b8c
to
86ba732
Compare
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 like the updated structure! It seems to me much more readable. Just a couple of extra requests for the API doc of what is conditional stuff and nitpicks.
qiskit_experiments/library/tomography/fitters/postprocess_fit.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/tomography/fitters/postprocess_fit.py
Outdated
Show resolved
Hide resolved
cae9020
to
f048b87
Compare
Upgrades conditional_measurement_indices to work with multiple conditional measurement bases indices, rather than assuming only a single basis index is used when conditional.
f048b87
to
f7f9476
Compare
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.
Separation of conditional prep and meas indices seems to make data structure cleaner. I don't have enough time to track every numeral details, so my review strategy is to trust unittest and focus on handling of data. It seems to me like the unittest for conditional prep and meas are too simplified (though current test looks good to check edge case), because it conditions on the qubit of a single qubit circuit. I guess most of fitter code is not covered with it.
# Run experiment | ||
backend = AerSimulator(seed_simulator=7172) | ||
exp = ProcessTomography(QuantumCircuit(1), backend=backend) | ||
exp.analysis.set_options(conditional_measurement_indices=[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.
Is this practical test case? What is the purpose of this test? Seems like there is nothing to fit.
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 equivalent to measurement tomography, and the reconstructed conditional states should be the measurement POVMs for each measurement basis (ie the Z, X and Y eigenstate projectors as Choi channels with input dim =2 output dim = 1), which is what the test is checking
rhos_i = [] | ||
cons = [] | ||
for i in range(num_circ_components): | ||
for j in range(num_tomo_components): |
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 is the expected size of these elements? Quadruple loop seems quite heavy, but of course it depends on the element size.
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.
It is what it is. Though of course depends on the tomography you are doing. You are essentially trading the dimension of the optimization problem for the number of components. eg for 3 qubits, instead of 1 8x8 matrix, you can have 2 4x4 matrices, or 4 2x2 or 8 1x1 etc
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 okey, the outer loops are on conditional elements so there is actually no loop without conditioning.
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 Chris. This looks good to me.
Summary
This adds support for performing conditional fragment tomography to the tomography experiments.
Details and comments
Conditional tomography involves conditioning on specific clbit values of the input circuit (if it contains clbits), and/or specific tomography basis measurement outcomes and performing the tomographic fit on each conditional component or fragment. The sum of all fragments weighted by the fragment probabilities should be the unconditional fit (either a state or channel). In the case of the cvxpy fitters trace and trace preserving constraints may be applied to this conditional fit from the sum since, for example, a trace preserving channel does not need to have trace preserving components (eg a measurement channel).