[Obs] 4.5 - High-level API #4392

mpharrigan · 2021-08-06T19:01:39Z

Provide two very similar high-level entries into the observable measurement framework. This provides more sensible defaults for some of the execution details and can return nicer values

    df = measure_observables_df(
        circuit,
        [observable],
        cirq.Simulator(),
        stopping_criteria='variance',
        stopping_criteria_val=1e-3 ** 2,
    )

measure_grouped_settings lets you control how settings are grouped and returns "raw" data in one container (BitstringAccumulator) per group. This level of control is probably desirable for people seriously executing and expensive experiment on the device. The high-level API makes some simplifying assumptions

The qubits are the union of circuit and observable qubits
We will group the settings using a GROUPER_T function, by default "greedy"
We'll always initialize settings into the |00..00> state, so you can just provide PauliStrings
You'll probably be using one of the two built-in stopping criteria and can just provide a name and a value instead of grokking these objects.
(for the second entrypoint) you really don't care about how things were grouped and executed, you'd just like the observable values in a dataframe table

mpharrigan · 2021-08-06T19:02:19Z

I'm still adding some tests, annotations, and going over the docstrings but feel free to take a look in the mean time

95-martin-orion

Nothing major for now, just a couple of cleanup suggestions.

cirq-core/cirq/work/observable_measurement.py

mpharrigan · 2021-08-17T21:40:11Z

Thanks for the initial look @95-martin-orion .

Since you wrote #3910, I'd like to make this PR get us basically all the way there. I'll re-work, but let me know if you have any ideas or want to set up a video call to sync

mpharrigan · 2021-08-17T22:47:43Z

f92fc65 implements the API from #3910

It includes the tests verbatim. One's failing because I think I'm doing the sweep unrolling wrong. I haven't gone through the docstring/input validation again. Those are copied from the original PR by @95-martin-orion

Current thinking on the APIs:

lowest: measure_grouped_observables. The user has to group things and deal with InitObsSetting instead of PauliString and gets back raw data (List[BistringAccumulators]) that are grouped like the grouping
questionable medium: measure_observables will group for you and take PauliString. It still returns the raw data (List[BitstringAccumulator]) that are grouped like the grouping
high, data-science edition: measure_observables_df: get a dataframe with all the results
high, Sampler API edition: Sampler.sample_expectation_values takes PauliSums (as well as PauliStrings) and will do the summation for you. It only lets you use RepetitionsStoppingCriteria. It returns values nested by input sweep index, input pauli sum index.

I'm not super happy with the "medium" version's return type. Maybe it can be a flat list of ObservableMeasuredResult

95-martin-orion · 2021-08-18T22:53:19Z

I think this is a reasonable philosophy for the API - especially now that the Sampler code is (mostly) relying on the public interface of Observable Measurements. Probably can update the docstring there to point specifically at measure_observables_df as the data science alternative.

mpharrigan · 2021-08-23T21:04:42Z

cirq-core/cirq/work/observable_measurement.py

    qubits = sorted({q for ms in grouped_settings.keys() for q in ms.init_state.qubits})
    qubit_to_index = {q: i for i, q in enumerate(qubits)}

    needs_init_layer = _needs_init_layer(grouped_settings)
    measurement_param_circuit = _with_parameterized_layers(circuit, qubits, needs_init_layer)
-    grouped_settings = {
-        _pad_setting(max_setting, qubits): settings


the call to _pad_setting has been moved to right near where it's actually needed in _get_params_for_setting. This was causing a subtle bug with the new test I added that uses a particularly bad grouper: it puts each observable in its own group. However, if you pad the keys in this bad grouping you start to get collisions. We'll keep the user's input max_settings whenever we're using it as a key anywhere and just "pad" when we need the actual circuit parameters.

mpharrigan · 2021-08-23T21:13:49Z

Lint check is for documenting exceptions. Some offline discussion has built some consensus that we should follow the style guide, especially:

You should not document exceptions that get raised if the API specified in the docstring is violated (because this would paradoxically make behavior under violation of the API part of the API)

and not document "input validation" checks in the docstring.

95-martin-orion

I think this pass captures all of my remaining concerns. Feel free to punt to another reviewer for final approval if I'm unavailable when these are resolved.

95-martin-orion · 2021-08-23T21:15:06Z

cirq-core/cirq/work/observable_measurement.py

+    from dataclasses import dataclass as json_serializable_dataclass
+else:
+    from cirq.protocols import json_serializable_dataclass


Does the Cirq json_serializable_dataclass break type checking? Using two different classes under the same name feels really unstable to me.

It breaks type checking. I introduced #4391 to fix this, but it isn't merged yet.

Got it. I stand by my comment, though - this should wait until #4391 is merged and use the fix it provides.

Ok. We definitely have this pattern in Cirq already as the linked pr demonstrates. It's really a drawback of mypy more than anything else; the decorator works fine

95-martin-orion · 2021-08-23T21:23:26Z

cirq-core/cirq/work/observable_measurement_data.py

+    from dataclasses import dataclass as json_serializable_dataclass
+else:
+    from cirq.protocols import json_serializable_dataclass


Ditto here for using different types during typechecking.

cirq-core/cirq/work/observable_measurement_test.py

cirq-core/cirq/work/sampler.py

95-martin-orion · 2021-08-23T21:36:43Z

cirq-core/cirq/work/observable_measurement_data.py

+        """
+        record = dataclasses.asdict(self)
+        del record['circuit_params']
+        record.update(**self.circuit_params)


Do you think we should check for collisions between record and self.circuit_params here? It seems unlikely that someone would use overlapping parameter names, but if they did it would be really tricky to spot what went wrong.

You're right. What should the behavior be in this case? an error? that could be pretty annoying especially if it's after a long data collection run. Prefix the new key names with "circuit_params."? Probably. Should we only do the prefixing if a collision would have happened, or always?

prefixed names with "circuit_params." if and only if any of the circuit params would collide

Agreed that we shouldn't return an error, though personally I'm in favor of consistency (i.e. always prefix).

changed to always prefixing

mpharrigan · 2021-08-23T22:15:25Z

@95-martin-orion PTAL if you're still around :)

95-martin-orion

Approval conditioned on the changes discussed. Thanks!

CirqBot · 2021-08-26T21:02:40Z

Automerge cancelled: A required status check is not present.

Missing statuses: ['Build docs', 'Build protos', 'Coverage check', 'Doc test', 'Format check', 'Lint check', 'Misc check', 'Notebook formatting', 'Pytest MacOS (3.7)', 'Pytest MacOS (3.8)', 'Pytest MacOS (3.9)', 'Pytest Ubuntu (3.7)', 'Pytest Ubuntu (3.8)', 'Pytest Ubuntu (3.9)', 'Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Type check', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage']

Provide two very similar high-level entries into the observable measurement framework. This provides more sensible defaults for some of the execution details and can return nicer values ```python df = measure_observables_df( circuit, [observable], cirq.Simulator(), stopping_criteria='variance', stopping_criteria_val=1e-3 ** 2, ) ``` ------------------ `measure_grouped_settings` lets you control how settings are grouped and returns "raw" data in one container (`BitstringAccumulator`) per group. This level of control is probably desirable for people seriously executing and expensive experiment on the device. The high-level API makes some simplifying assumptions - The qubits are the union of circuit and observable qubits - We will group the settings using a `GROUPER_T` function, by default "greedy" - We'll always initialize settings into the `|00..00>` state, so you can just provide `PauliString`s - You'll probably be using one of the two built-in stopping criteria and can just provide a name and a value instead of grokking these objects. - (for the second entrypoint) you really don't care about how things were grouped and executed, you'd just like the observable values in a dataframe table

[Obs] 4.5 - High-level API

dcb59f3

mpharrigan added the area/expectation-value label Aug 6, 2021

mpharrigan requested a review from cduck as a code owner August 6, 2021 19:01

mpharrigan assigned 95-martin-orion Aug 6, 2021

mpharrigan requested review from vtomole and a team as code owners August 6, 2021 19:01

mpharrigan requested a review from maffoo August 6, 2021 19:01

google-cla bot added the cla: yes Makes googlebot stop complaining. label Aug 6, 2021

mpharrigan requested review from 95-martin-orion and removed request for maffoo August 6, 2021 19:05

95-martin-orion requested changes Aug 9, 2021

View reviewed changes

cirq-core/cirq/work/observable_measurement.py Outdated Show resolved Hide resolved

cirq-core/cirq/work/observable_measurement.py Outdated Show resolved Hide resolved

cirq-core/cirq/work/observable_measurement.py Show resolved Hide resolved

CirqBot added size: XL lines changed >1000 size: M 50< lines changed <250 and removed size: XL lines changed >1000 labels Aug 12, 2021

mpharrigan mentioned this pull request Aug 16, 2021

Observable Measurement - 4 - Sampling Loop #3647

Closed

Sampler API

f92fc65

mpharrigan added 2 commits August 17, 2021 15:52

Remove stringy stopping criteria

0aa1e51

Checkpoint container

66fe36a

mpharrigan added 6 commits August 23, 2021 10:20

some updates

45e9d74

Merge remote-tracking branch 'origin/master' into obs-4.5-api

ea6bbfa

fix some of the tests

a65e143

Merge remote-tracking branch 'origin/master' into obs-4.5-api

500aa75

Tidy

2b94b7b

Tests, CI

b78d7ee

mpharrigan commented Aug 23, 2021

View reviewed changes

95-martin-orion requested changes Aug 23, 2021

View reviewed changes

mpharrigan added 2 commits August 23, 2021 14:56

"flat_params" to reduce confusion

25cdc1c

Prevent clobbering

deb9ea6

always prefix

7cd0c12

95-martin-orion approved these changes Aug 23, 2021

View reviewed changes

mpharrigan added 3 commits August 26, 2021 10:21

Merge remote-tracking branch 'origin/master' into obs-4.5-api

6974b7a

Stop using json_serializable_dataclass

6d42a61

lint

307c58e

mpharrigan added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Aug 26, 2021

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Aug 26, 2021

Merge branch 'master' into obs-4.5-api

9bc0a66

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Aug 26, 2021

mpharrigan added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Aug 26, 2021

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Aug 26, 2021

Merge branch 'master' into obs-4.5-api

c8e9bc1

CirqBot merged commit 0dce2c1 into quantumlib:master Aug 26, 2021

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Aug 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Obs] 4.5 - High-level API #4392

[Obs] 4.5 - High-level API #4392

mpharrigan commented Aug 6, 2021

mpharrigan commented Aug 6, 2021

95-martin-orion left a comment

mpharrigan commented Aug 17, 2021

mpharrigan commented Aug 17, 2021 •

edited

Loading

95-martin-orion commented Aug 18, 2021

mpharrigan Aug 23, 2021

mpharrigan commented Aug 23, 2021

95-martin-orion left a comment

95-martin-orion Aug 23, 2021

mpharrigan Aug 23, 2021 •

edited

Loading

95-martin-orion Aug 23, 2021

mpharrigan Aug 23, 2021

95-martin-orion Aug 23, 2021

95-martin-orion Aug 23, 2021

mpharrigan Aug 23, 2021

mpharrigan Aug 23, 2021

95-martin-orion Aug 23, 2021

mpharrigan Aug 23, 2021

mpharrigan commented Aug 23, 2021

95-martin-orion left a comment •

edited

Loading

CirqBot commented Aug 26, 2021

[Obs] 4.5 - High-level API #4392

[Obs] 4.5 - High-level API #4392

Conversation

mpharrigan commented Aug 6, 2021

mpharrigan commented Aug 6, 2021

95-martin-orion left a comment

Choose a reason for hiding this comment

mpharrigan commented Aug 17, 2021

mpharrigan commented Aug 17, 2021 • edited Loading

95-martin-orion commented Aug 18, 2021

Choose a reason for hiding this comment

mpharrigan commented Aug 23, 2021

95-martin-orion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpharrigan Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpharrigan commented Aug 23, 2021

95-martin-orion left a comment • edited Loading

Choose a reason for hiding this comment

CirqBot commented Aug 26, 2021

mpharrigan commented Aug 17, 2021 •

edited

Loading

mpharrigan Aug 23, 2021 •

edited

Loading

95-martin-orion left a comment •

edited

Loading