feat: added tomography result class - #863
Conversation
☂️ Code Coverage
Overall Coverage
New Files
Modified FilesNo covered modified files...
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
david-pl
left a comment
david-pl
left a comment
There was a problem hiding this comment.
There's a tricky issue with normalization, and the frozen dataclass feels off.
|
|
||
| import numpy as np | ||
|
|
||
| BASES = ("X", "Y", "Z") |
There was a problem hiding this comment.
You convert this to a set a few times and all other usage, as far as I can tell, would also work if you just made this a set.
| BASES = ("X", "Y", "Z") | ||
|
|
||
|
|
||
| def _density_matrix_from_bloch(bloch: Mapping[str, float]) -> np.ndarray: |
There was a problem hiding this comment.
Hm, this is tricky: mixed states are not normalized, but if you leave things unnormalized then you can get a bloch vector with norm > 1 leading to invalid density matrices:
import numpy as np
from bloqade.analysis.tomography import TomographyResult
shots = {basis: np.array([0]) for basis in ("X", "Y", "Z")}
result = TomographyResult(shots)
print(np.linalg.eigvalsh(result.density_matrix)) # negative eigenvalue is not validA solution to fix the above would be to normalize if norm > 1, but that doesn't guarantee that the norm of a Bloch vector for a mixed state is too large so long as it's < 1.
There was a problem hiding this comment.
I see, yeah this is tricky. I was looking into other quantum computing libraries, and a common pattern is to have some kind of "fitter" that finds the most likely physically valid density matrix given the measurement outcomes (examples: Forest-Benchmarking, Qiskit). We could do something similar, and have a DensityMatrix object instead? And add methods for computing fidelity to other states to that class (such as def fidelity_bloch(target_bloch: np.ndarray)).
To implement this DensityMatrix object, we could probably reuse the existing QuantumState class (
There was a problem hiding this comment.
This sounds like a good idea. I'd be happy to refactor the QuantumState class, but it might lead to breaking changes, which I'm not sure we'll want. Also, I'm not sure how much work this will be.
An alternative would be to document the behavior here saying that things aren't normalized for now and then leave the rest for a follow-up ticket. I'll leave that up to you.
There was a problem hiding this comment.
I think we can keep the TomographyResult class as is and define the "fitter" methods on the class as alternative constructors. And regarding interoperability with the QuantumState class, in a future PR we can inherit from it (won't do so now as a lot of the methods in the QuantumState class are not implemented)
| return overlap + 2.0 * math.sqrt(max(det_product, 0.0)) | ||
|
|
||
|
|
||
| @dataclass(frozen=True, init=False) |
There was a problem hiding this comment.
Having a frozen dataclass with a mutable field (you can mutate the density matrix, and you set it in the __init__) and a custom __init__ is odd, I think. I'd suggest to at least remove the frozen=True. Might also make sense to make the __init__ a __post_init__. Or, maybe even don't make it a dataclass at all if you need the __init__?
Backport results for aecd13fSucceeded:
|
2 participants
Wanted to create a PR to start the discussion of adding a
TomographyResultclass, in order to reconstruct a quantum state from basis measurementsI think it's related to this issue: #529