-
Notifications
You must be signed in to change notification settings - Fork 23
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
Refactor linear inversion state tomography and notebook #4
Conversation
Clean up by using `vec` and experiment settings in place of `n_qubit_pauli_basis`
Why am I getting transposed?
forest_qcvv/tomography.py
Outdated
dim = 2 ** len(qubits) | ||
rho = rho.reshape((dim, dim)) | ||
# TODO: how did this get transposed? | ||
return rho.T |
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 think at one point (bitbucket) rho was vectorized, the transpose was possibly used then. The state rho is Hermitan so the dagger transpose should not change things but transpose by itself will.
can test with a simple 1 qubit state rho = (1/2) (x X + yY + z Z)
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.
Y = 1j * PROJ_PLUS - 1j * PROJ_MINUS is this correct? np.round(Y, 2)
Out[4]:
array([[0.-0.j, 0.+1.j],
[0.+1.j, 0.-0.j]]) |
In the interest of keeping this PR from becoming ginormous and forever-taking, I've shimmed over iterative_mle and estimate_variance to flip between the previous and current data structures. I'll open a follow-up ticket for refactoring that to use This PR depends on rigetti/pyquil#552 |
If PROJ_PLUS is the plus eigenvector of X, then no this isn't correct. Notice Y isn't Hermitian so something must have gone wrong. I'm not sure what the context is to say more. |
"Z_EFFECTS = [PROJ_ZERO, PROJ_ONE]\n", | ||
"X_EFFECTS = [PROJ_PLUS, PROJ_MINUS]\n", | ||
"X = PROJ_PLUS - PROJ_MINUS\n", | ||
"Y = 1j * PROJ_PLUS - 1j * PROJ_MINUS\n", |
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.
Found the context. Notice that this would simply be i*X since X = ProjPlus - ProjMinus. The top right element of Y should be negative and the bottom left positive.
"PLUS = np.array([[1], [1]]) / np.sqrt(2)\n", | ||
"PROJ_PLUS = PLUS @ PLUS.T.conj()\n", | ||
"PROJ_MINUS = ID - PROJ_PLUS\n", | ||
"Z_EFFECTS = [PROJ_ZERO, PROJ_ONE]\n", |
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.
Should proj_one be negative here? That would make it more analogous to X_EFFECTS because in the x basis the X_EFFECTS are PROJ_ZERO, -PROJ_ONE
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.
Sorry, I don't understand here. xeffects are proj_plus and proj_minus, not proj_zero and -proj_one.
by analogy, there's no minus sign in either z_effects or x_effects as currently written
"P10 = np.kron(PROJ_ONE, PROJ_ZERO)\n", | ||
"P11 = np.kron(PROJ_ONE, PROJ_ONE)\n", | ||
"ID_2Q = P00 + P01 + P10 + P11\n", | ||
"ZZ_EFFECTS = [P00, P01, P10, P11]" |
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 thing here, I would think P01 and P10 should be negative.
"\n", | ||
"qc = get_qc('2q-pyqvm')\n", | ||
"from forest_qcvv.compilation import basic_compile\n", | ||
"qc.compiler.quil_to_native_quil = basic_compile\n", |
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 would be good to explain what this achieves with a comment.
"\n", | ||
"# Two qubit defs\n", | ||
"P00 = np.kron(PROJ_ZERO, PROJ_ZERO)\n", | ||
"P01 = np.kron(PROJ_ZERO, PROJ_ONE)\n", |
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.
We need to be careful about standardizing qubit ordering in tensor products. I had done everything this way for process tomography before noticing that state tomography was reversed. I think process tomography currently uses the convention opposite of the one used here. I prefer the convention here.
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.
yeah this is a problem. With this refactor, I've given linear_inv_state_estimate
an explicit argument: qubits
and the resulting rho will be tensored according to this list
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.
specifically for these, I'm going to leave them as-is. As you've probably discovered, this is just copied from test_state_tomography.py
. We can clean those up in a follow-on pr
from pyquil.operator_estimation import ExperimentSetting, \ | ||
TomographyExperiment as PyQuilTomographyExperiment, ExperimentResult, measure_observables | ||
from pyquil.paulis import sI, sX, sY, sZ, PauliSum, PauliTerm | ||
from pyquil.unitary_tools import lifted_pauli |
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 in pyquil master?
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.
no, it's in rigetti/pyquil#552
@@ -100,6 +113,19 @@ class TomographyData: | |||
"""number of shots used to calculate the `expectation`""" | |||
|
|||
|
|||
def shim_pyquil_results_to_TomographyData(program, qubits, results: List[ExperimentResult]): | |||
return TomographyData( | |||
in_ops=[r.setting.in_operator for r in results[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.
Not sure if significant, but in_ops is currently Optional[List[str]]
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.
yeah yeah. The shimming is a little rough-and-ready, under the basis that it will become unnecessary after we refactor all the functions. In my tests, this wasn't an actual problem since in_ops
is just a record of what was done and isn't used in the analysis routines. If it becomes a problem we can make the shim more robust by casting to strings
:param operators: A list of ndarray-like objects. | ||
:return: An ndarray that when applied to a vector of expectation values returns a vectorized | ||
estimate of the state. | ||
:param results: A tomographically complete list of results. |
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.
missing qubits in docstring
Z_EFFECTS = [PROJ_ZERO, PROJ_ONE] | ||
X_EFFECTS = [PROJ_PLUS, PROJ_MINUS] | ||
X = PROJ_PLUS - PROJ_MINUS | ||
Y = 1j * PROJ_PLUS - 1j * PROJ_MINUS |
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.
Needs to be corrected as above, though not used.
PLUS = np.array([[1], [1]]) / np.sqrt(2) | ||
PROJ_PLUS = PLUS @ PLUS.T.conj() | ||
PROJ_MINUS = ID - PROJ_PLUS | ||
Z_EFFECTS = [PROJ_ZERO, PROJ_ONE] |
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 seems to match what was there before but I'm not convinced it's right
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.
yeah all this stuff I took verbatim from what was there before. Will have to do some investigating on who wrote it to make sure it's correct
addressed non-(defining projectors correctly) review items |
gonna merge and open a follow-on ticket |
fixes #7
TODO: