Skip to content

Measurement Results Refactor #3868

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

Closed
smitsanghavi opened this issue Feb 27, 2021 · 7 comments
Closed

Measurement Results Refactor #3868

smitsanghavi opened this issue Feb 27, 2021 · 7 comments
Labels
area/measurements kind/design-issue A conversation around design status/rfc-needed This needs an RFC document. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on

Comments

@smitsanghavi
Copy link
Collaborator

Is your design idea/issue related to a use case or problem? Please describe.
Based on the #3808, #3233, #3785 and other discussion around changing the measurement Results, proposing some high-level design ideas below for discussion. Once a consensus is reached, this will be developed into an RFC.

Names below are only descriptive, final names might be better.

Describe your design idea/issue

Proposal based on inheritance (inheritance was decided in Cirq Cynque):

  • AbstractResults interface
    • Would have measurement_dict, flat_measurements, dataframe (not data) accessors.
      • Implemented as lazily evaluated properties in the concrete classes
  • KeyedResults Interface inherits from AbstractResults
    • Natively tracks the measurement keys
    • Has from_flat_measurements method that also needs key -> index_list mapping (can be computed from the circuit)
  • QuditResults Interface inherits from AbstractResults
    • Stores measurements as qudits (including qubits)
    • [maybe] Has from_iq_points method that also needs some form of distribution/separation boundaries in IQ-space.
  • class MeasurementDictResults(KeyedResults, QuditResults)
    • [tangential] The current Dict will change considering nested and repeated CircuitOperations. We will add tuple measurement keys
      • Dict might evolve into a more hierarchical tree structure if we want to automatically group at higher CircuitOperation levels
  • class FlatMeasurementResults(QuditResults)
  • class PandasResults(KeyedResults, QuditResults)
    • Almost always costlier except when user wants to interact with Pandas for convenience
    • The structure of the DataFrame might also change based on the above changes in the Measurement Dict.
  • class IQPointsResults/RawResults(KeyedResults)
    • If we decide not to pursue IQ Points right now, QuditResults interface can also be removed.
    • These is a fundamentally different and a higher dimensional data. IQ Points can be converted to any of the types above (given the boundaries to use), but not the other way round.

Strategy pattern (built on another suggestion in #3808 and much simpler):

  • Define a MeasurementStorageStrategy enum
  • Instead of all the inheritance, pass this enum to the current Results class
  • Results will use this enum to determine what data structure to use for native measurement storage
  • Accessors for other storage types are still available as lazily evaluated properties.
    • Since we'll need different logic for different strategies, this implementation will be longer than the inheritance above, but no more complex.

[Future work/Classical data] Take in MeasurementKey->ClassicalRegister mapping to populate classical data during the run in addition to populating the native data structure. FlatMeasurementResults might take an Index->ClassicalRegister mapping

@smitsanghavi smitsanghavi added kind/design-issue A conversation around design triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Feb 27, 2021
@daxfohl
Copy link
Collaborator

daxfohl commented Feb 28, 2021

A third option to toss into the hat: just use arrays or bitarrays everywhere until as late as possible.

  1. Change simulators' ActOnArgs.log_of_measurement_results to be a bitarray, and measurement just appends.
  2. Also change samplers' sampling behavior to append into a bitarray.
  3. Have Result type constructor take a bitarray and an in-order list of measurement keys from the circuit, and store them as private member fields.
  4. Allow Result to provide the bitarray directly for perf-sensitive analysis.
  5. If user looks up measurement by key, only then create a dictionary using the aforementioned key list and memoize it for future lookups.

Pros:

  • One way to do things, so avoids the potential code bloat of abstract classes or strategy patterns.
  • Performant by default.
  • Consistent with the behavior of real hardware (IIUC).

Cons:

  • May be harder to fit with Feed-Forward if ActOnArgs does not have measurement key dictionary.
  • May be impossible to fit with flow control / dynamic circuits since the measurement keys would not have a guaranteed order that we could feed into the Result constructor.
  • Would need to settle on how to handle multi-bit measurements (separate arrays, slicing, etc).

@smitsanghavi
Copy link
Collaborator Author

@Strilanc @maffoo @balopat, thoughts?

I'll work towards expanding on these options in an RFC format

@balopat
Copy link
Contributor

balopat commented Mar 10, 2021

Feedback on Cirq Cynque:

  1. Focus on the abstract interface / UX
  2. IQ points - it is unclear yet whether this should stay in the same Result class coming back from the same method or have a separate method and result type - let's dig in for more pros/cons
  3. Strategy pattern doesn't seem that great a fit here

Quick sketch:

  1. step
abstract class Result: 
   
 @abstract_property
   _measurements
    
   @abstract
   def measurements_as_dataframe(): 

   @abstract
   def measurements_as_dict(): 
...
  1. use case driven design (qsim, simulators, etc.)

Then we'll figure out how default implementations will be layed out.

@balopat balopat added triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on status/rfc-needed This needs an RFC document. and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Apr 21, 2021
@95-martin-orion
Copy link
Collaborator

CC @MichaelBroughton to flag for possible cleanup before Cirq 1.0.

maffoo added a commit that referenced this issue Jan 26, 2022
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 #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.

Review: @95-martin-orion
@mpharrigan
Copy link
Collaborator

cc @maffoo I think we've made the critical change of moving Result to an abstract base class. Going to mark this as "after-1.0" since we've done the backwards-compatibility-concern part

@mpharrigan
Copy link
Collaborator

cc #5152

Do we actually want all the other types of results? I think not really. What do folks think about closing this issue and opening specific issues for specific result types or functionality that is missing?

@maffoo
Copy link
Contributor

maffoo commented Apr 20, 2022

+1 from me to close this and file separate issues for additional specialized result types if needed.

rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
…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
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/measurements kind/design-issue A conversation around design status/rfc-needed This needs an RFC document. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on
Projects
None yet
Development

No branches or pull requests

6 participants