Convert cirq.Result to ABC and add ResultDict implementation #4806

maffoo · 2022-01-06T19:46:12Z

This converts cirq.Result into an abstract base class which defines the interface provided by measurement result objects. We introduce a new class cirq.ResultImpl which is the default implementation of this result type, based on a dict mapping string keys to array data. This will allow us to provide alternate implementations as needed, for example one could have a result that is based on a DataFrame and only converts to measurement dict on demand (cf #4774), or a result type that is based on experimental data (such as raw IQ data) and allows accessing this raw data while also converting to the standard measurements dict on demand (see #3868 for other possible Result implementations). Making this separation will also simplify the implementation of repeated measurement keys (#4274), since it will allow us to define the new interface for accessing data from repeated keys separately from the implementation of how this data is stored.

maffoo · 2022-01-06T19:46:53Z

@daxfohl, FYI

daxfohl · 2022-01-06T21:38:06Z

I think before doing this, let's make sure it's necessary. It may be possible that there's a unique internal representation that allows us to do everything we need, with high performance in all cases. Like perhaps if we model more closely what happens in hardware, that would be sufficient and performant for all the simulation logic as well. If that's the case then that one implementation would be sufficient and the abstraction would be unnecessary. (Not saying that there definitely is such a representation, just suggesting we should explore that approach first.)

maffoo · 2022-01-06T21:55:52Z

I think it's unlikely we'll have a single representation. At the very least, for data coming from hardware we sometimes have each measurement represented as a pair of I,Q values (if doing discrimination in software) and sometimes as a single bit (if doing discrimination onboard the control electronics). Most simulators would likely produce bits for measurements, so would never want to use the IQ representation.

95-martin-orion · 2022-01-11T16:06:04Z

I think it's unlikely we'll have a single representation {...}

I'll second this comment, with the added note that IQ points are specific to a particular subset of QC hardware designs. In order for Cirq to support IQ points and remain applicable to other hardware, multiple representations are all but required.

95-martin-orion · 2022-01-11T16:14:32Z

That said, as implemented this is a breaking change. To avoid this we could instead name the abstract class AbstractResult and use Result for the implementation - or if Result is not the desired implementation name, then rename it and make Result a deprecated, empty subclass of the new class:

class NewResult(AbstractResult):
    # ...contents of old Result class

@deprecated
class Result(NewResult):
    pass  # acts identically to NewResult, for backwards-compatibility

95-martin-orion

Couple of smaller comments in addition to the breaking-change concern.

95-martin-orion · 2022-01-11T16:16:25Z

cirq-core/cirq/study/result.py

@@ -80,52 +82,35 @@ def _key_to_str(key: TMeasurementKey) -> str:
    return ','.join(str(q) for q in key)


-class Result:
+class Result(abc.ABC):


Update class docstring to reflect new behavior.

95-martin-orion · 2022-01-11T16:23:18Z

cirq-core/cirq/study/result.py

+        return ResultImpl(params=self.params, measurements=all_measurements)
+
+
+class ResultImpl(Result):


With an eye towards #3868, it may be preferable to name this implementation something that clearly indicates that its "default" storage type is keyed measurements (as opposed to raw values, pandas DataFrame, or something more exotic like IQ points).

I'm certainly open to renaming this, but not sure what a good name would be. Any specific suggestions?

The simplest would be "KeyedResultImpl" or "MeasurementKeyResultImpl" - the others could then be e.g. "RawResultImpl" and "DataFrameResultImpl".

maffoo · 2022-01-14T21:04:13Z

Re: backwards compatibility, I'm personally inclined to keep the simple name Result as the name of the interface, and take the hit of minor breakage (I wish we had taken this road for Circuit vs AbstractCircuit, but I guess that ship has sailed). I think the breakage should be pretty small since most code that uses results does not construct them directly (other than simulator implementations and hardware interfaces) and so most users should not be affected by this change. The advantage we get is that all functions currently annotated as returning cirq.Result will immediately work with any result type, without having to change their annotations, including important interfaces like cirq.Sampler. (In the case of Circuit we probably still have a long tail of functions that should be changed to take AbstractCircuit, and it'd be nice to avoid that.)

In addition, to ease the transition I've added a Result.__new__ method that will construct a ResultImpl so calls to the Result constructor can go through a standard deprecation cycle.

95-martin-orion · 2022-01-14T23:08:38Z

cirq-core/cirq/study/result.py

-    """
+    """The results of multiple executions of a circuit with fixed parameters."""
+
+    def __new__(cls, *args, **kwargs):


Huh, this is a really neat workaround.

95-martin-orion

I think I'm sold on the ResultImpl vs AbstractResult discussion, especially with the fix to avoid breaking Result(...) calls. Left a comment on names for ResultImpl, but other than that this is ready.

maffoo · 2022-01-25T17:28:20Z

I've renamed the concrete implementation class ResultDict. I prefer Result*** over ***Result because it keeps things alphabetically close, but I don't have a strong preference here. Opening this up for brief bike-shedding from maintainers if anyone feels strongly, but I'd like to merge today if possible.

The `cirq.Result` constructor is deprecated as of quantumlib/Cirq#4806. Instead, we should use a concrete type like `cirq.ResultDict`. This changes `StimSampler` to use `cirq.ResultDict` when available, to avoid deprecation warnings. I'm also curious to know what is the support policy for stimcirq supporting different pyle versions. This workaround can be removed whenever support for cirq < 0.14 is dropped.

@95-martin-orion

…lib#4806) This converts cirq.Result into an abstract base class which defines the interface provided by measurement result objects. We introduce a new class cirq.ResultDict which is the default implementation of this result type, based on a dict mapping string keys to array data. This will allow us to provide alternate implementations as needed, for example a result type that is based on a DataFrame and only converts to measurement dict on demand (cf quantumlib#4774), or a result type that is based on experimental data (such as raw IQ data) and allows accessing this raw data while also converting to the standard measurements dict on demand (see quantumlib#3868 for other possible Result implementations). Making this separation will also simplify the implementation of repeated measurement keys (quantumlib#4274), since it will allow us to define the new interface for accessing data from repeated keys separately from the implementation of how this data is stored. Review: @95-martin-orion

@95-martin-orion

…lib#4806) This converts cirq.Result into an abstract base class which defines the interface provided by measurement result objects. We introduce a new class cirq.ResultDict which is the default implementation of this result type, based on a dict mapping string keys to array data. This will allow us to provide alternate implementations as needed, for example a result type that is based on a DataFrame and only converts to measurement dict on demand (cf quantumlib#4774), or a result type that is based on experimental data (such as raw IQ data) and allows accessing this raw data while also converting to the standard measurements dict on demand (see quantumlib#3868 for other possible Result implementations). Making this separation will also simplify the implementation of repeated measurement keys (quantumlib#4274), since it will allow us to define the new interface for accessing data from repeated keys separately from the implementation of how this data is stored. Review: @95-martin-orion

maffoo requested review from cduck, ColemanCollins, dabacon, gmauricio, ma5x, nakardo, pschindler, vtomole, wcourtney and a team as code owners January 6, 2022 19:46

CirqBot added the size: L 250< lines changed <1000 label Jan 6, 2022

maffoo requested a review from 95-martin-orion January 6, 2022 19:46

maffoo mentioned this pull request Jan 6, 2022

Change Sampler methods to return Sequence instead of List #4807

Merged

Convert cirq.Result to ABC and add ResultImpl implementation

e4799e0

maffoo force-pushed the u/maffoo/result-impl branch from d91186c to e4799e0 Compare January 7, 2022 00:12

95-martin-orion added the BREAKING CHANGE For pull requests that are important to mention in release notes. label Jan 11, 2022

95-martin-orion requested changes Jan 11, 2022

View reviewed changes

maffoo added 2 commits January 14, 2022 12:34

Update docstrings

53dfda3

Add backwards compatibility for Result() constructor

931e506

95-martin-orion reviewed Jan 14, 2022

View reviewed changes

95-martin-orion approved these changes Jan 14, 2022

View reviewed changes

Rename ResultImpl -> ResultDict

b5e7586

maffoo changed the title ~~Convert cirq.Result to ABC and add ResultImpl implementation~~ Convert cirq.Result to ABC and add ResultDict implementation Jan 25, 2022

maffoo and others added 3 commits January 25, 2022 09:30

Merge branch 'master' into u/maffoo/result-impl

9019a3d

Merge branch 'master' into u/maffoo/result-impl

c3c9354

Merge branch 'master' into u/maffoo/result-impl

9919433

maffoo merged commit b0519b3 into master Jan 26, 2022

maffoo deleted the u/maffoo/result-impl branch January 26, 2022 20:13

maffoo mentioned this pull request Feb 4, 2022

Use cirq.ResultDict in StimSampler when available quantumlib/Stim#197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert cirq.Result to ABC and add ResultDict implementation #4806

Convert cirq.Result to ABC and add ResultDict implementation #4806

maffoo commented Jan 6, 2022

maffoo commented Jan 6, 2022

daxfohl commented Jan 6, 2022

maffoo commented Jan 6, 2022

95-martin-orion commented Jan 11, 2022

95-martin-orion commented Jan 11, 2022

95-martin-orion left a comment

95-martin-orion Jan 11, 2022

maffoo Jan 14, 2022

95-martin-orion Jan 11, 2022

maffoo Jan 14, 2022

95-martin-orion Jan 14, 2022

maffoo commented Jan 14, 2022 •

edited

Loading

95-martin-orion Jan 14, 2022

95-martin-orion left a comment

maffoo commented Jan 25, 2022

		return ResultImpl(params=self.params, measurements=all_measurements)


		class ResultImpl(Result):

Convert cirq.Result to ABC and add ResultDict implementation #4806

Convert cirq.Result to ABC and add ResultDict implementation #4806

Conversation

maffoo commented Jan 6, 2022

maffoo commented Jan 6, 2022

daxfohl commented Jan 6, 2022

maffoo commented Jan 6, 2022

95-martin-orion commented Jan 11, 2022

95-martin-orion commented Jan 11, 2022

95-martin-orion left a comment

Choose a reason for hiding this comment

95-martin-orion Jan 11, 2022

Choose a reason for hiding this comment

maffoo Jan 14, 2022

Choose a reason for hiding this comment

95-martin-orion Jan 11, 2022

Choose a reason for hiding this comment

maffoo Jan 14, 2022

Choose a reason for hiding this comment

95-martin-orion Jan 14, 2022

Choose a reason for hiding this comment

maffoo commented Jan 14, 2022 • edited Loading

95-martin-orion Jan 14, 2022

Choose a reason for hiding this comment

95-martin-orion left a comment

Choose a reason for hiding this comment

maffoo commented Jan 25, 2022

maffoo commented Jan 14, 2022 •

edited

Loading