-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add Sampler API for expectation values #3910
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
Add Sampler API for expectation values #3910
Conversation
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 would have marked this "comment" but then the "see changes since last reviewed" doesn't work, so you get the angry red X)
It surprised me to see this implemented this way. This is halfway towards the sophisticated cirq.work
set of routines. I would have expected this to serve as a simple, reference implementation. Specifically, I was surprised to see grouping and parameterized circuits showing up here, although I'm delighted that some of the functionality in cirq.work
was useful. In that vein, we should converge on how to turn the results from a sampler to expectation values. I'm not so sure about the current bit twiddling in this PR.
The reason why I would have expected a very simple "reference implementation" is that if you wanted any degree of sophistication, it would probably make sense to just wrap the high-level cirq.work
entry functions. That is, this function body would be like two lines. Of course that high-level API isn't merged yet. You can preview it e.g. https://github.com/quantumlib/Cirq/pull/3647/files#diff-da3dfc139c4c6d4a898a2ad1b919f0e3c72880da2c2606288077bb15ae80b630R562 here. (Line 562). The upside is the signature (given the rest of the default arguments) matches the API you've proposed here.
Edit: none of this to say that the current PR is bad. It looks pretty straightforward.
cirq/work/sampler.py
Outdated
obs_pstrings: List[ops.PauliString] = [] | ||
for psum in psums: | ||
pstring_aggregate = {} | ||
for pstring in psum: | ||
pstring_aggregate.update(pstring.items()) | ||
obs_pstrings.append(ops.PauliString(pstring_aggregate)) |
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.
what is this section doing? Factor out into a private helper function?
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.
The purpose of this is to get InitObsSettings
for each PauliSum
. Since observables_to_settings
only accepts PauliString
s, I perform this conversion for each PauliSum
provided.
If it makes sense to you, I could fold this into observables_to_settings
and have it accept a mix of PauliString
and PauliSum
as observables.
cirq/work/sampler.py
Outdated
for sweep_idx, pr in enumerate(study.to_resolvers(input_params)): | ||
subseries = series | ||
for k in pr: | ||
subseries = subseries & (samples[k] == pr.value_of(k)) | ||
int_results = samples.loc[subseries, 'z'] | ||
for setting in grouped_settings: | ||
obs_idx = obs_settings.index(setting) | ||
psum = psums[obs_idx] | ||
ev = 0 | ||
for pstr in psum: | ||
sum_val = 0 | ||
for int_result in int_results: | ||
val = 1 | ||
for q in pstr.qubits: | ||
bit_idx = num_qubits - qmap[q] - 1 | ||
if (int_result >> bit_idx) & 1: | ||
val *= -1 | ||
sum_val += val | ||
ev += sum_val * pstr.coefficient | ||
results[sweep_idx][obs_idx] = ev / num_samples |
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 section is very difficult to read and I suspect much slower than a vectorized numpy solution would be. Maybe factoring parts into helpfully-named helper functions or comments could go a long way?
Since you're already using some of the cirq.work
functionality, consider using observable_measurement_data._stats_from_measurements
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.
Done. Also split off a helper function, but with _stats_from_measurements
there's not much left to split off.
In addition to the changes noted above, I added a plug in the method description for the full observable measurements toolkit, including the selling points listed in #2781. |
For Simulators that implement the Sampler interface, have you considered just using the analytic expectation values and drawing samples from a binomial distribution? |
I suppose we could do this? It seems orthogonal to this PR, though - those simulators have access to this implementation for now (since As an aside: what is the use case for this? IIUC, such a simulator could implement |
from cirq.work import group_settings_greedy, observables_to_settings | ||
from cirq.work.observable_measurement import ( | ||
_get_params_for_setting, | ||
_with_parameterized_layers, |
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 bad practice to import private functions from other modules. It breaks naming assumptions. We should either change the name to indicate that it is no longer private or remove the import (or create a new method that is public in those modules).
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 agree, although this behavior is consistent with the observable measurements imports (all files are under cirq/work
):
Cirq/cirq/work/observable_measurement_data.py
Lines 23 to 29 in 845836a
from cirq.work.observable_settings import ( | |
InitObsSetting, | |
_max_weight_observable, | |
_max_weight_state, | |
_MeasurementSpec, | |
zeros_state, | |
) |
Should I send a "prequel" PR that fixes this before moving forward with this one?
param_resolver: 'cirq.ParamResolverOrSimilarType', | ||
mask: Optional[pd.Series] = None, | ||
) -> pd.Series: | ||
"""Generates a 'mask' for a given parameterization of a sample. |
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.
What is a mask for a parameterization? I think this should be explained.
|
||
This is a minimal implementation for measuring observables, and is best | ||
reserved for simple use cases. For more complex use cases, consider | ||
upgrading to `cirq/work/observable_measurement.py`. Additional features |
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.
Change to the user visible name instead of the source code name, such as cirq.work.measure_grouped_settings
number of repetitions | ||
- Readout error symmetrization and mitigation | ||
|
||
This method can be run on any device or simulator that supports circuit |
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.
Consider reordering or clarifying this. It's unclear which "this method" you are referring to (observable measurement or sample_expectation_values). Maybe consider putting the pointing to observable measurement at the end. I think that makes more sense to explain what this function does before pointing out alternatives.
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.
Clarified which method this refers to. I prefer to keep this paragraph at the top of the docstring, since all but the simplest use cases can benefit from using the Observable Measurement toolkit instead.
To that end, there's an argument to be made for dropping this API altogether: it doesn't provide functionality outside what will be possible with the Observable Measurement toolkit, and potentially distracts users from the "correct" tools for getting expectation values. As it stands, it primarily serves as a simple reference implementation, as pointed out above.
qubits = ops.QubitOrder.DEFAULT.order_for(program.all_qubits()) | ||
qmap = {q: i for i, q in enumerate(qubits)} | ||
num_qubits = len(qubits) | ||
psums = ( |
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.
Optional: consider avoiding abbreviations and use pauli_sums to be more clear. Optional since this does make the pauli_string_aggregate variable name kind of long.
Discussed offline: putting this on hold in lieu of full Observable Measurement toolkit. To avoid distracting users from that toolkit, we may end up dropping this PR. |
ping @95-martin-orion - if the decision is to go with the OM toolkit, maybe you could team up with Matt to clean up those PRs? :) |
Can confirm that this was the decision we reached. Reading #3647 it looks like the next PR is still in development - @mpharrigan feel free to assign me to review on that when it comes out. |
Closing this PR as discontinued. |
Stage 2 of the Expectation Value API (#3492). This is a rewrite of #3700.
This PR introduces the sample_expectation_values method and its default implementation, which injects gates to ensure measurement occurs in the correct basis. Hardware and software samplers can use this method as-is or implement their own version for custom behavior (e.g. server-side calculation of expectation values).