-
Notifications
You must be signed in to change notification settings - Fork 347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Operator estimation #682
Operator estimation #682
Conversation
I'm going through the tests. I'm going to revert most of the pauli changes since they aren't the point of this PR and are potentially breaking changes. I'll keep them on a branch for later proposal |
return ps | ||
qubits = self.get_qubits() | ||
|
||
return ''.join(self[q] for q in qubits) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good change! For example, if we did (sZ(1) * sX(0)).pauli_string()
, we'd get 'XZ'
, which disagrees with the QUIL convention of qubit ordering. With this change, we now get 'ZX'
, which agrees with the QUIL convention of qubit ordering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait. I don't want the behavior to change. Why did this cause the behavior to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because the code in master does qubits = [qubit for qubit, _ in qubit_term_mapping.items()]
, which in the example I gave above, will produce [0, 1]
(the opposite of the conventional qubit ordering in Quil). Using qubits = self.get_qubits()
instead as you do here gives the correct [1, 0]
. Note though that if we had instead defined our PauliTerm as sX(0) * sZ(1)
, we would again get [0, 1]
using qubits = self.get_qubits()
(so maybe in a future PR, we might want to fix this too?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but qubit_term_mapping was dict(self.operations_as_set())
which would be random order python for python < 3.6. In >= 3.6, the full call is dict(frozenset(OrderedDict()))
which might be ordered (???) in which case it would share ordering with self.get_qubits()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, but otherwise changes look good to me. There might be more changes to operator_esimation, but this can happen in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Miserly with the inline code comments. e.g.
measure_observables
has a number of steps without any explanation. - Is is worth supporting analog IQ values if we're estimating and analog value?
pyquil/api/_quantum_computer.py
Outdated
@@ -199,7 +199,8 @@ def run_symmetrized_readout(self, program: Program, trials: int) -> np.ndarray: | |||
return results | |||
|
|||
@_record_call | |||
def run_and_measure(self, program: Program, trials: int) -> Dict[int, np.ndarray]: | |||
def run_and_measure(self, program: Program, trials: int, active_reset=False) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's design philosophy I suppose but why have the complication of an active_reset
as a kwarg
and not just have users be explicit in the program
whether they want to use RESET
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably outside the scope of this PR, I admit; but
- qubits are still reset if you don't use
RESET
. It just uses a different protocol - more discoverable / documentable this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove it from this PR, as we should probably have a full discussion and do more input validation (ie what happens if a user uses both active_reset
and adds a RESET
instruction)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it from here but kept it as a kwarg for measure_observables
main function in this PR.
I'm going to rename
to better reflect these data structures. I'm pretty happy with the former, but will happily take suggestions for the latter |
…tion-refactor # Conflicts: # pyquil/api/_quantum_computer.py
create data structures to handle programs of the form (initial_pauli, state_prep_program, measure_in_a_basis). Utilities for grouping terms into those that share a tensor product basis.
also includes sundry changes to
PauliTerm
to make it marginally more sensible.This should make writing tomography-like programs or variational algorithms easier as it provides a higher-level API for running things besides
qc.run()
. I imagine the functionmeasure_observables
to live (in some capacity) on theQuantumComputer
object