Implement hermitian and unitary analysis and canonicalization pass #475
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
|
|
||
|
|
||
| @statement(dialect=dialect) | ||
| class Rot(CompositeOp): |
There was a problem hiding this comment.
Is there a reason Rot doesn't have any hermitian trait?
There was a problem hiding this comment.
No not really. I can add it, but it's only hermitian if the angle is 0, in which case it's actually just the identity. So the use-case is just very limited, which is why I skipped it.
There was a problem hiding this comment.
Oh, this also applies to U3, PhaseOp and ShiftOp (maybe a few more). I feel like it's a bit of an overkill to have the hermitian analysis in place here, as it's always only the case that they are hermitian when the exponent is 0 and therefore the operator is just the identity. What do you think @johnzl-777 ?
As a side note: I generally think this should be a rewrite pass, which just replaces the operator by the identity directly. But we don't really have any such simplification passes in-place right now. For example, simplifying multiplications with identity operators would be another such candidate.
There was a problem hiding this comment.
I see, I think your proposal for a rewrite makes sense here instead of burdening other statements with the hermitian information. I suppose I'm just nervous how much of a downside we could get if it turns out something IS hermitian (this 0 exponent case) and we don't catch it.
I imagine it wouldn't be the worst thing in the world and if we knew we'd have something spitting out 0-exponents we'd most likely have thought to include that rewrite in the first place.
Co-authored-by: John Long <[email protected]>
|
@johnzl-777 thanks for the review!
Yes, my idea was to get this done first and then the rewrite for |
|
Okay, after the first round of review I've now added a proper lattice implementation for both analyses. They are just
and similar for Ready for another round of review! |
My understanding of the top element in the type lattice is there is the possibility it COULD be any other lattice element but there's insufficient information to narrow it down. In that case I think something like |
|
|
||
|
|
||
| @statement(dialect=dialect) | ||
| class Rot(CompositeOp): |
There was a problem hiding this comment.
I see, I think your proposal for a rewrite makes sense here instead of burdening other statements with the hermitian information. I suppose I'm just nervous how much of a downside we could get if it turns out something IS hermitian (this 0 exponent case) and we don't catch it.
I imagine it wouldn't be the worst thing in the world and if we knew we'd have something spitting out 0-exponents we'd most likely have thought to include that rewrite in the first place.
Co-authored-by: John Long <[email protected]>
@johnzl-777 I agree. While working out the solution, however, I noticed a problem, which I now realize is why I wrote things like this initially: Any basic operator statement that is not hermitian or not unitary represents this by an absence of the respective trait. For example, I solved this by adding a simple check for |
3 participants
Still needs better testing, but works in principle.
I'm abusing
EmptyLatticehere a bit. Technically, I think you'd need a lattice to representHermitianNotHermitianNotAnOperatorand similar for unitary.
I'm just doing this now with
True,Falseandmissing.