Add Observable container type for Estimator
#11594
Conversation
chriseclectic
added
priority: high
mod: primitives
|
One or more of the the following people are requested to review this:
|
The intent is that an Estimator can retrieve the basis terms in an observables array from the `terms` attribute and validate them itself, rather than hard code the allowed terms here. If we do still want to hardcode this in the classes we could add it back, but the current version of expecting subclasses to override ALLOWED_BASIS doesn't work with the current implementation of class methods which done return the subclass type from methods like reshape etc
This changes the mapping key type to be `(tuple[int], str)` instead of str, to allow for sparse representations in the future. Eg instead of `"XIIIY" you can have `((0, 5,), "XY")`. Also removes validation of allowed characters in the strings. Still need to update unit tests if we go with this.
|
I have a question about the following commit message.
|
|
|
| from qiskit.quantum_info import Pauli, SparsePauliOp | ||
|
|
||
|
|
||
| ObservableKey = "tuple[tuple[int, ...], str]" # pylint: disable = invalid-name |
There was a problem hiding this comment.
It might be good to make a dataclass of ObservableKey and add a validation method for #11594 (comment).
There was a problem hiding this comment.
I prefer the name ObservableTerm here, but at the end of the day, it's just a name. I wonder if you can comment on the feasibility of leaving this as tuple[tuple[int, ...], str] for now and changing it to a class (eg as suggested by @t-imamichi) in the future? In terms of breaking changes and possible subclassing.
There was a problem hiding this comment.
Im mostly worried about this container being slow and memory intensive with all the nested classes. I did have a prototype using a class that function like a tuple (had getitem), but im a bit skeptical of validation other than via type since it might be slow. We can't validate the strings, so validation would be that qubits is a tuple with no duplicates and (if we want to enforce this) that the string is same lenght as qubits.
In the current version the design doesn't actually require the string to represent a bunch of operators for each qubit, its up to an estimator to decide what it does with it, but we are using it to represent a Pauli label
(side note: our reference estimator is going to be rather inefficient since it unpacks SparsePauliOps into this potentially much more memory heavy format, and then has to convert back to SparsePauliOp for its simulation)
There was a problem hiding this comment.
Thanks Chris, this looks like a strict improvement because it allows sparsity. I think we should continue to think about how an Estimator is supposed to express its abilities (eg can it estimate only Paulis, or more stuff?) and whether it would make sense to class-ify ObservableKey. One or both of these questions could be out-of-scope for this PR.
| from qiskit.quantum_info import Pauli, SparsePauliOp | ||
|
|
||
|
|
||
| ObservableKey = "tuple[tuple[int, ...], str]" # pylint: disable = invalid-name |
There was a problem hiding this comment.
I prefer the name ObservableTerm here, but at the end of the day, it's just a name. I wonder if you can comment on the feasibility of leaving this as tuple[tuple[int, ...], str] for now and changing it to a class (eg as suggested by @t-imamichi) in the future? In terms of breaking changes and possible subclassing.
|
|
||
| def __init__( | ||
| self, | ||
| data: Mapping[ObservableKey, complex], |
There was a problem hiding this comment.
Maybe we can and should brush this under the rug for now, but the dtype of the values is complex here, but the StatevectorEstimator is already setting evs to np.float64. If we could have some mechanism to determine tho common type of an array and use that to set the dtype in the DataBin, it would be nice.
There was a problem hiding this comment.
Also,
| data: Mapping[ObservableKey, complex], | |
| data: Mapping[ObservableKeyLike, complex], |
There was a problem hiding this comment.
I'm happy to change to float instead of complex (since any hermitian nqubit matrix can be decompose with real coefficients in Pauli basis). Im not sure different exisiting estimators are consistent with enforcing hermitian or not.
Currently for performance its not the inits job to convert Observable-key like, thats the coerce methods job. If you initialize with not an actual observable key validation should fail.
There was a problem hiding this comment.
In reference to preceding comment, Iean to Key instead of Term since this object is a mapping, and this type is literally the key type for the mapping. In operator speak my first impression of a term would probably be the key * value (coeff).
| Raises: | ||
| ValueError: If ``validate=True`` and the input observable-like is not valid. | ||
| """ | ||
| self._data = data |
There was a problem hiding this comment.
If we care about not sharing a reference to the internal thing:
| self._data = data | |
| self._data = dict(data.items()) |
There was a problem hiding this comment.
Its a trade off with that vs performance since this requires an iteration over the data at init
| qubits = tuple(range(len(key))) | ||
| key = (qubits, key) |
There was a problem hiding this comment.
This should have the same TODO as Pauli.
There was a problem hiding this comment.
This assumes we always mean "I" in a key to mean identity, which if we do we should be more explicit about (and in general with strings being single characters same length as qubits etc)
| self._num_qubits = num_qubits | ||
| return self._num_qubits | ||
|
|
||
| def validate(self): |
There was a problem hiding this comment.
This validation allows no terms, which I guess corresponds to the identity observable?
There was a problem hiding this comment.
Maybe we should fail if emtpy so identity has to be represented as ((0,), "I") or similar?
|
I should add that i am not a fan of the internal data structure of this class (or of previous ObservableArray values it is encapsulating). A python dict with tuple keys seems like it will scale poorly. Eg something that seems more scalable would be using 1D BitArray as mask of non-iden qubits, 1D float array as coeffs. The labels are the tricky part since they could change in size depending on the number of masked qubits (unless we encode a small number of allowed label terms). Ideally I would like to not enforce an internal representation with this class, but i am struggling to figure out how to do this in a practical way that can still be used by primitive implementers |
|
I'm going to close this now that we aren't rushed to put it into 1.0, it would be good to consider the questions raised by this PR and the review comments to come up with a nice proposal for 1.1 |
|
Reopening for 1.1 |
|
I'm bumping this out of the 1.1, which is my understanding of the intention of the primitives team from discussion with Ian. Fwiw, we may well want to work out new timescales together to unify this with #12282. |
|
See #12671, which will need to be completed before this PR can be closed. This PR may be closed via merge, or because a new PR supersedes it. |
Summary
This aims to add a concrete container class type for the items in an
ObservableArraywhich was an oversite when adding that container.This is important so that we can use this to abstract away the internal storage format of an observable from how it is constructed for Estimator inputs, and consumed by Estimator primitive developers.
Details and comments
Some open questions / ideas:
MappingorSequenceto define the developer API for how it is iterated over and accessed.ObservableArray[BaseObservable]andEstimatorPub[BaseObservable]as templated types?