Adding support for mixed basis during measurement#44
Conversation
ausbin
left a comment
ausbin
left a comment
There was a problem hiding this comment.
I think we should discuss how this works on a call before you work on this further, since I don't want to sink too much of your time into this code if it is using an unsound approach.
There was a problem hiding this comment.
This is a great test, can you change it to being an integration test?
Here is an example test you can imitate:
- program itself (equivalent to this file you've written, except the
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 - the "unit" test that calls the code and verifies the output: https://github.com/gt-tinker/qwerty/blob/main/qwerty_pyrt/python/qwerty/tests/integration_tests.py#L429-L436
| rebuild!(Basis, self, canonicalize) | ||
| } | ||
|
|
||
| pub fn strip_phases(self) -> Self { |
There was a problem hiding this comment.
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 ({'1'} >> {'1'} is an identity, whereas {'1'} >> {-'1'} is a Z gate
There was a problem hiding this comment.
For any recursive code you add, can you write a TODO saying e.g. // TODO: use gen_rebuild to this non-recursively? I don't want to make you worry about it right now, but tldr recursive code can cause compile-time stack overflows for deep ASTs. We have crazy macros to work around that, but it's not worth your time atm considering the qwerty_ast_to_mlir code is already comically recursive
There was a problem hiding this comment.
I don't know exactly why this change that is already in main is showing up in this diff. I was going to blame GitHub, but I can reproduce it with git diff origin/main...akshay/mixedbasis, yet not with git diff origin/main..akshay/mixedbasis (note the difference in number of periods). Honestly, I don't mean this disrespectfully, but I think the history for this mixedbasis branch is just convoluted. Let's just ignore this part of the diff for this PR (I've verified it's not an actual change versus main), but you can avoid it in the future by creating a fresh branch off main and cherry-picking your changes, or by rebasing against main, I believe.
Let me know if I'm being confusing here
| 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(); |
There was a problem hiding this comment.
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)
| } | ||
| } | ||
|
|
||
| pub fn factor_separable(self) -> Self { |
There was a problem hiding this comment.
Can you add a doc comment for this?
|
|
||
| let mut rows: Vec<Vec<Vector>> = Vec::with_capacity(vecs.len()); | ||
| for v in &vecs { | ||
| match v.clone().canonicalize() { |
There was a problem hiding this comment.
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
| return Basis::BasisLiteral { vecs, dbg }; | ||
| } | ||
|
|
||
| let mut letters: Vec<Vec<Vector>> = vec![Vec::new(); n]; |
There was a problem hiding this comment.
It would be helpful to leave a comment with an example describing what this variable is holding
| if !letters[i].iter().any(|l| l.approx_equal(atom)) { | ||
| letters[i].push(atom.clone()); |
There was a problem hiding this comment.
Dang, I am trying pretty hard, but I can't figure out what this is doing. Maybe we should discuss in a call
|
|
||
| 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)) |
There was a problem hiding this comment.
I almost get it I think, but I just can't figure out what this does. We should talk on a call I think
2 participants
Previously mixed bases such as:
and
were not working. This change introduces a factor_separable function, which takes in a BasisLiteral and returns a BasisTensor, that the existing lowering code already handles. Additionally, an update was made to the measure path to ignore phases.