-
Notifications
You must be signed in to change notification settings - Fork 7
Adding support for mixed basis during measurement #44
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
base: main
Are you sure you want to change the base?
Changes from all commits
8e5edbf
97e3692
97ac9f2
69acf09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| #!/usr/bin/env python3 | ||
| """ | ||
| Z (x) X separable basis, with per-vector phases at various angles. | ||
|
|
||
| {'0p','0m','1p','1m'} is separable: qubit 0 in Z, qubit 1 in X. | ||
|
|
||
| """ | ||
|
|
||
| from qwerty import * | ||
|
|
||
|
|
||
| @qpu | ||
| def no_phase() -> bit[2]: | ||
| return '00' | {'0p', '0m', '1p', '1m'}.measure | ||
|
|
||
| @qpu | ||
| def all_phase() -> bit[2]: | ||
| return '00' | {'0p'@90, '0m'@90, '1p'@90, '1m'@90}.measure | ||
|
|
||
|
|
||
| @qpu | ||
| def with_phase_180() -> bit[2]: | ||
| return '00' | {'0p', '0m', '1p', '1m'@180}.measure | ||
|
|
||
|
|
||
| @qpu | ||
| def with_phase_45() -> bit[2]: | ||
| return '00' | {'0p', '0m', '1p', '1m'@45}.measure | ||
|
|
||
|
|
||
| @qpu | ||
| def with_phase_60() -> bit[2]: | ||
| return '00' | {'0p', '0m', '1p', '1m'@60}.measure | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| # All four must agree (angle-independent under measurement). | ||
| print("no phase: ", histogram(no_phase(shots=512))) | ||
| print("all phase: ", histogram(all_phase(shots=512))) | ||
| print("with phase @180:", histogram(with_phase_180(shots=512))) | ||
| print("with phase @45: ", histogram(with_phase_45(shots=512))) | ||
| print("with phase @60: ", histogram(with_phase_60(shots=512))) |
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For any recursive code you add, can you write a TODO saying e.g. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2023,6 +2023,73 @@ impl Basis { | |
| rebuild!(Basis, self, canonicalize) | ||
| } | ||
|
|
||
| pub fn strip_phases(self) -> Self { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may be missing something, but I don't see why we should remove vector phases. I added code to remove (outer) phases for span checking, since that information is redundant with respect to span ( |
||
| match self { | ||
| Basis::BasisLiteral { vecs, dbg } => Basis::BasisLiteral { | ||
| vecs: vecs.into_iter().map(|v| v.canonicalize().normalize()).collect(), | ||
| dbg, | ||
| }, | ||
| Basis::BasisTensor { bases, dbg } => Basis::BasisTensor { | ||
| bases: bases.into_iter().map(Basis::strip_phases).collect(), | ||
| dbg, | ||
| }, | ||
| other => other, | ||
| } | ||
| } | ||
|
|
||
| pub fn factor_separable(self) -> Self { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a doc comment for this? |
||
| let Basis::BasisLiteral { vecs, dbg } = self else { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is cool. I have never used this syntax before, respect. |
||
| return self; | ||
| }; | ||
| if vecs.len() < 2 { | ||
| return Basis::BasisLiteral { vecs, dbg }; | ||
| } | ||
|
|
||
| let mut rows: Vec<Vec<Vector>> = Vec::with_capacity(vecs.len()); | ||
| for v in &vecs { | ||
| match v.clone().canonicalize() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we mandate that canonicalization happens on the vectors before this (in the doc comment for this method), then we can skip this clone & canonicalization, since the vectors will already be canonicalized |
||
| Vector::VectorTensor { qs, .. } => { rows.push(qs); } | ||
| _ => return Basis::BasisLiteral { vecs, dbg }, | ||
| } | ||
| } | ||
|
|
||
| let n = rows[0].len(); | ||
| if rows.iter().any(|r| r.len() != n) { | ||
| return Basis::BasisLiteral { vecs, dbg }; | ||
| } | ||
|
|
||
| let mut letters: Vec<Vec<Vector>> = vec![Vec::new(); n]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be helpful to leave a comment with an example describing what this variable is holding |
||
|
|
||
| for row in &rows { | ||
| for (i, atom) in row.iter().enumerate() { | ||
| if !letters[i].iter().any(|l| l.approx_equal(atom)) { | ||
| letters[i].push(atom.clone()); | ||
|
Comment on lines
+2065
to
+2066
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dang, I am trying pretty hard, but I can't figure out what this is doing. Maybe we should discuss in a call |
||
| } | ||
| } | ||
| } | ||
|
|
||
| let expected: Vec<Vec<Vector>> = letters | ||
| .clone() | ||
| .into_iter() | ||
|
Comment on lines
+2072
to
+2073
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to delay the clone here, i.e., |
||
| .multi_cartesian_product() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably add some kind of short-circuit check somewhere to bypass this optimization if Do we really need to allocate something this big? |
||
| .collect(); | ||
|
|
||
| let is_factorable = rows.len() == expected.len() | ||
| && rows.iter().zip(&expected).all(|(r, e)| { | ||
| r.len() == e.len() && r.iter().zip(e).all(|(a, b)| a.approx_equal(b)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I almost get it I think, but I just can't figure out what this does. We should talk on a call I think |
||
| }); | ||
|
|
||
| if !is_factorable { | ||
| return Basis::BasisLiteral { vecs, dbg }; | ||
| } | ||
|
|
||
| let bases = letters | ||
| .into_iter() | ||
| .map(|vecs| Basis::BasisLiteral { vecs, dbg: dbg.clone() }) | ||
| .collect(); | ||
| Basis::BasisTensor { bases, dbg }.canonicalize() | ||
| } | ||
|
|
||
| pub(crate) fn canonicalize_rewriter(self) -> Self { | ||
| match self { | ||
| Basis::BasisLiteral { vecs, dbg } => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -518,7 +518,8 @@ fn try_basis_as_primitive(basis_elems: &Vec<Basis>) -> Option<qwerty::PrimitiveB | |
| /// of phases which correspond one-to-one with any vectors that have | ||
| /// hasPhase==true. | ||
| fn ast_basis_to_mlir(basis: &Basis) -> MlirBasis { | ||
| let basis_elements = basis.to_explicit().canonicalize().to_vec(); | ||
| let basis_elements = basis.to_explicit().canonicalize().to_vec().into_iter() | ||
| .flat_map(|b| b.factor_separable().to_vec()).collect(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason we shouldn't do this factoring as part of canonicalization? Here's what we mean by "canonicalization", by the way: https://sunfishcode.github.io/blog/2018/10/22/Canonicalization.html Do you see what I mean? If not let me know The only problem I can think of is that it might confuse the type checker when doing span checking, but hopefully in that case, we just fix the span checker. (I can do that if it happens) |
||
|
|
||
| let prim_basis = try_basis_as_primitive(&basis_elements); | ||
| let (elems, phases) = if let Some(prim_basis) = prim_basis { | ||
|
|
@@ -1155,7 +1156,7 @@ fn ast_qpu_expr_to_mlir( | |
| explicit_indices, | ||
| pad_indices, | ||
| tgt_indices, | ||
| } = ast_basis_to_mlir(basis); | ||
| } = ast_basis_to_mlir(&basis.clone().strip_phases()); | ||
| assert!(pad_indices.is_empty()); | ||
| assert!(tgt_indices.is_empty()); | ||
| assert_eq!(explicit_indices.len(), dim); | ||
|
|
||
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 a great test, can you change it to being an integration test?
Here is an example test you can imitate:
if __name__ == '__main__'code is inside a function calledtest()instead (i usually make it take ashotsargument): https://github.com/gt-tinker/qwerty/blob/main/qwerty_pyrt/python/qwerty/tests/integ/meta/float_expr.py