detector/observable annotation dialect #603
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
david-pl
left a comment
david-pl
left a comment
There was a problem hiding this comment.
The code looks good, just one comment regarding the type.
On a high-level though: since you are making this part of the squin dialect group, what should the behavior here be when running any other interpreter / emit, such as PyQrack or cirq codegen? Is this just expected to error?
Or, even more generally: do we expect all statements in a dialect group to be supported by all interpreters we have for that dialect group?
src/bloqade/annotate/stmts.py
Outdated
| @statement(dialect=dialect) | ||
| class SetDetector(ConsumesMeasurementResults): | ||
| coordinates: ir.SSAValue = info.argument( | ||
| type=kirin_types.Tuple[kirin_types.Int | kirin_types.Float] |
There was a problem hiding this comment.
Shouldn't this be an IList? Is the length here known?
Judging from the tests, this could be IListType[types.Float, types.Literal(2)].
Do we need to support Int here?
There was a problem hiding this comment.
this type is wrong.
- tuple[int] means it only have one element....
- I think the intention here is the concern of sometimes ppl do:
(0, 3.2, 5)but I think we should actually not allow it moving forward (error when validate in the future)
There was a problem hiding this comment.
- I think kirin's
Tupletype actually aliases to a vararg tuple. - I agree, so then this type should be changed to ilist.
There was a problem hiding this comment.
yeah, so the right way to hint it (if you really want to use tuple) is:
types.Tuple[types.Float, ...] which is equivalent to types.Tuple[types.Vararg(types.Float)]
Also, for 2. we can discuss if we want that UX. I don't have strong opinion on that.
There was a problem hiding this comment.
@kaihsin why do we want to prohibit variable length tuple for the coordinates? My understanding is they just exist to make visualization easier, it doesn't affect any functionality.
Judging from the tests, this could be IListType[types.Float, types.Literal(2)].
@david-pl I think the only reason I cooked up tests with two integer coordinates is because I don't think I've ever seen a 3D coordinate used. That being said, I imagine there must be more complicated codes where this would be desirable
There was a problem hiding this comment.
@kaihsin's point with the tuple length was that ´Tuple[Int]´ indicates a tuple with only a single element. So the correct typing for variable length here would be ´Tuple[Int, ...]´.
I see, then disregard the comment on the length.
I'll admit the current structure was inspired by @rafaelha 's proposal, I'm completely open to creating a separate kernel/group similar to what I agree it's not great to have set up the expectation squin works with all these backends and potentially break it by introducing a new dialect. That being said, I assume that if a user is using annotate they aren't really gunning to simulate it on the existing pyqrack/cirq backends? |
Yeah, that's why I'm also not sure here. So I don't have any strong opinion here. I suppose the error you get (something like |
Luckly, set_detector and observable have no effect on the quantum circuit (they just annotate expected error-free outputs and requested logical results of the circuit). So PyQrack and cirq can safely ignore these instructions. If you really want to, you can compute the requested set_detector and observable XORs of the measurement results and provide these as additional simulator output but the user could just as easily use stim to do this. |
This brings in SetDetector and SetObservable into bloqade-circuit. Historically this was the
physicaldialect in another project but I've gone ahead and renamed itannotate.