Change how pauli_keys are represented in CummulativeObservableAnnotation #982
Conversation
| _pauli_keys.append((cirq.LineQubit(int(k[1:])), k[0])) | ||
| else: | ||
| _pauli_keys.append(tuple(k)) | ||
| self.pauli_keys = frozenset(_pauli_keys) |
There was a problem hiding this comment.
What if a user has code for k in op.pauli_keys: if k.startswith("..."): ... ? This would break that.
There was a problem hiding this comment.
I guess you mean that changing the type of pauli_keys from str to tuple[cirq.QiD, str] is an issue? The problem I'm trying to solve is that there are no reference to the "stim qubit id" when we are in cirq, and there is no guarantee that these ids will mean anything useful when we convert the circuit back from stim to cirq.
We could keep the type str and do something like X(1,1) and then interpret k[1:] as a tuple/Qid. That would not impact the snippet you showed, but we could certainly come up with another example snippet that would break (for example if the user has code that does int(k[1:]) it would break...). I think this kind of string interpretation might be worse, but I'm game if you prefer it? I don't really see a clean way to achieve this change... But if you have some preferred format, I'm happy to implement it. Fwiw, since this is a pretty recent addition to stimcirq, I don't think it's gotten too much adoption yet, so making a change now is unlikely to impact anyone at this point.
Anyway, again I'm open to whatever you think is the best solution here. Let me know!
There was a problem hiding this comment.
The main thing is that you must make the changes in a way that doesn't break existing code. Existing code is likely to depend on the type of public fields, so you cannot change their type. You can add other fields, though.
There was a problem hiding this comment.
Ok, I tried a version of this. Let me know what you think!
I don't love it, but the previous representation is insufficient because if we modify the cirq circuit the "stim qubit id" of these pauli keys gets out of sync. Therefore, we need to convert the qubit representation to the actual coordinates to keep track of this.
Let me know what you think! Or if you have suggestions on how to achieve this in another way.
Also regarding the backward compatibility, I added some ugly logic in the init to support
Iterable[str]to make sure thattest_json_backwards_compat_exactstill passes, but alternatively we could also just avoid that logic and not be backward compatible here. I don't think we have many (or any?) serialized cirq_circuit with non-deterministic observables yet (certainly none that we care about!). But anyway let me know what you prefer.