Skip to content
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

TrialResults seems to assume only boolean results #3002

Closed
dabacon opened this issue May 13, 2020 · 9 comments
Closed

TrialResults seems to assume only boolean results #3002

dabacon opened this issue May 13, 2020 · 9 comments
Labels
area/qudits area/result complexity/medium introduces/modifies 3-5 concepts, takes max up to a month for an advanced contributor good for learning For beginners in QC, this will help picking up some knowledge. Bit harder than "good first issues" kind/feature-request Describes new functionality skill-level/advanced One or more of the areas need a solid understanding. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on

Comments

@dabacon
Copy link
Collaborator

dabacon commented May 13, 2020

Documentation says this but we think it should work for three outcome or more measurements.

We should

  1. verify it works for >2 dimensional outcomes
  2. update doc strings
@maffoo maffoo changed the title TrailResults seems to assume only boolean results TrialResults seems to assume only boolean results May 13, 2020
@kevinsung
Copy link
Collaborator

This at least will need to be changed:

value.big_endian_bits_to_int(m_vals) for m_vals in val

@cduck
Copy link
Collaborator

cduck commented May 13, 2020

Workaround: Use sampler.run().measurements instead of sampler.run().data or sampler.sample() until this is fixed.

This looks like something I missed (I even reviewed the PR that added TrialResult.data). The issue is TrialResult.data, TrialResult.histogram, TrialResult.multi_measurement_histogram, and TrialResults._json_dict_ all assume TrialResults.measurements is binary when converting the measurements dictionary to another form.

Here's an example:

import numpy as np
import cirq

class PlusOneGate(cirq.SingleQubitGate):
    def _qid_shape_(self):
        return (3,)
    def _unitary_(self):
        return np.array([[0, 0, 1],
                         [1, 0, 0],
                         [0, 1, 0]])
    def _circuit_diagram_info_(self, args):
        return '[+1%3]'
PlusOne = PlusOneGate()

q = cirq.LineQid.range(4, dimension=3)
c = cirq.Circuit(
    PlusOne.on_each(*q),
    PlusOne.on_each(q[0], q[2]),
    cirq.measure(q[0], key='a'),
    cirq.measure(*q[1:], key='b'),
)
print(c)
#0 (d=3): ───[+1%3]───[+1%3]───M('a')───
#
#1 (d=3): ───[+1%3]────────────M('b')───
#                              │
#2 (d=3): ───[+1%3]───[+1%3]───M────────
#                              │
#3 (d=3): ───[+1%3]────────────M────────

samp = cirq.Simulator()
results = samp.run(c, repetitions=2)

print(results.measurements)
# Correct qutrit measurements:
#{'a': array([[2],
#       [2]], dtype=uint8), 'b': array([[1, 2, 1],
#       [1, 2, 1]], dtype=uint8)}

print(results.data)
print(samp.sample(c, repetitions=2))  # Same as samp.run().data
# Incorrect when converted to a data frame:
#   a  b
#0  1  7
#1  1  7

print(results.histogram(key='b'))
# Incorrect generated histogram:
# Counter({7: 2})

@cduck
Copy link
Collaborator

cduck commented May 13, 2020

Currently, TrialResult doesn't know the qid shape of its data so these methods can't do the right thing. To fix this, it needs to store a qid shape (a tuple of qubit dimensions: Tuple[int, ...]) for each measurement key and every sampler that supports qudits needs to specify the shapes when they create TrialResults.

@maffoo
Copy link
Contributor

maffoo commented May 13, 2020

Instead of storing the qid shapes in TrialResult, I'd suggest we just store the list of qid's themselves. These will include the shapes, of course, but this also makes TrialResult more self-documenting since you can interpret the measurements without referring back to the measurement gate in the circuit to know what qubits were measured.

@smitsanghavi
Copy link
Collaborator

FWIW, this concern was raised in the original PR and it was assumed that TrialResult has access to the qid shape #1811 (comment).

+1 to including the qids themselves in TrialResult.

@kevinsung
Copy link
Collaborator

@cduck Actually the JSON serialization does support qudits. For instance, adding

assert cirq.read_json(json_text=cirq.to_json(results)) == results

to the end of your example works.

@cduck
Copy link
Collaborator

cduck commented May 14, 2020

You are right @kevinsung. I missed that _json_dict_ checks if any measurement result values are not 0/1 and uses numpy's array serialization instead.

@balopat balopat added area/qudits area/result complexity/medium introduces/modifies 3-5 concepts, takes max up to a month for an advanced contributor good for learning For beginners in QC, this will help picking up some knowledge. Bit harder than "good first issues" kind/feature-request Describes new functionality skill-level/advanced One or more of the areas need a solid understanding. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on labels Sep 18, 2020
CirqBot pushed a commit that referenced this issue Feb 7, 2022
Adds a `ClassicalDataStore` class so we can keep track of which qubits are associated to which measurements.

Closes #3232. Initially this was created as part 14 (of 14) of https://tinyurl.com/cirq-feedforward to enable qudits in classical conditions, by storing and using dimensions of the measured qubits when calculating the integer value of each measurement when resolving sympy expressions. However it may have broader applicability.

This approach also sets us up to more easily add different types of measurements (#3233, #4274). It will also ease the path to #3002 and #4449., as we can eventually pass this into `Result` rather than the raw `log_of_measurement_results` dictionary. (The return type of `_run` will have to be changed to `Sequence[C;assicalDataStoreReader]`.

Related: #887, #3231 (open question @95-martin-orion whether this closes those or not)

This PR contains a `ClassicalDataStoreReader` and `ClassicalDataStoreBase` parent "interface" for the `ClassicalDataStore` class as well. This will allow us to swap in different representations that may have different performance characteristics. See #3808 for an example use case. This could be done by adding an optional `ClassicalDataStore` factory method argument to the `SimulatorBase` initializer, or separately to sampler classes.

(Note this is an alternative to #4778 for supporting qudits in sympy classical control expressions, as discussed here: https://github.com/quantumlib/Cirq/pull/4778/files#r774816995. The other PR was simpler and less invasive, but a bit hacky. I felt even though bigger, this seemed like the better approach and especially fits better with our future direction, and closed the other one).

**Breaking Changes**:
1. The abstract method `SimulatorBase._create_partial_act_on_args` argument `log_of_measurement_results: Dict` has been changed to `classical_data: ClassicalData`. Any third-party simulators that inherit `SimulatorBase` will need to update their implementation accordingly.
2. The abstract base class `ActOnArgs.__init__` argument `log_of_measurement_results: Dict` is now copied before use. For users that depend on the pass-by-reference semantics (this should be rare), they can use the new `classical_data: ClassicalData` argument instead, which is pass-by-reference.
@95-martin-orion
Copy link
Collaborator

Hey @daxfohl, did #4781 end up resolving this?

@daxfohl
Copy link
Contributor

daxfohl commented Mar 28, 2022

Yes. That gets passed in from the OperationTarget.log_of_measurement_results property, which uses get_digits, which is integral not boolean.

return {str(k): list(self.classical_data.get_digits(k)) for k in self.classical_data.keys()}

rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
…mlib#4781)

Adds a `ClassicalDataStore` class so we can keep track of which qubits are associated to which measurements.

Closes quantumlib#3232. Initially this was created as part 14 (of 14) of https://tinyurl.com/cirq-feedforward to enable qudits in classical conditions, by storing and using dimensions of the measured qubits when calculating the integer value of each measurement when resolving sympy expressions. However it may have broader applicability.

This approach also sets us up to more easily add different types of measurements (quantumlib#3233, quantumlib#4274). It will also ease the path to quantumlib#3002 and quantumlib#4449., as we can eventually pass this into `Result` rather than the raw `log_of_measurement_results` dictionary. (The return type of `_run` will have to be changed to `Sequence[C;assicalDataStoreReader]`.

Related: quantumlib#887, quantumlib#3231 (open question @95-martin-orion whether this closes those or not)

This PR contains a `ClassicalDataStoreReader` and `ClassicalDataStoreBase` parent "interface" for the `ClassicalDataStore` class as well. This will allow us to swap in different representations that may have different performance characteristics. See quantumlib#3808 for an example use case. This could be done by adding an optional `ClassicalDataStore` factory method argument to the `SimulatorBase` initializer, or separately to sampler classes.

(Note this is an alternative to quantumlib#4778 for supporting qudits in sympy classical control expressions, as discussed here: https://github.com/quantumlib/Cirq/pull/4778/files#r774816995. The other PR was simpler and less invasive, but a bit hacky. I felt even though bigger, this seemed like the better approach and especially fits better with our future direction, and closed the other one).

**Breaking Changes**:
1. The abstract method `SimulatorBase._create_partial_act_on_args` argument `log_of_measurement_results: Dict` has been changed to `classical_data: ClassicalData`. Any third-party simulators that inherit `SimulatorBase` will need to update their implementation accordingly.
2. The abstract base class `ActOnArgs.__init__` argument `log_of_measurement_results: Dict` is now copied before use. For users that depend on the pass-by-reference semantics (this should be rare), they can use the new `classical_data: ClassicalData` argument instead, which is pass-by-reference.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
…mlib#4781)

Adds a `ClassicalDataStore` class so we can keep track of which qubits are associated to which measurements.

Closes quantumlib#3232. Initially this was created as part 14 (of 14) of https://tinyurl.com/cirq-feedforward to enable qudits in classical conditions, by storing and using dimensions of the measured qubits when calculating the integer value of each measurement when resolving sympy expressions. However it may have broader applicability.

This approach also sets us up to more easily add different types of measurements (quantumlib#3233, quantumlib#4274). It will also ease the path to quantumlib#3002 and quantumlib#4449., as we can eventually pass this into `Result` rather than the raw `log_of_measurement_results` dictionary. (The return type of `_run` will have to be changed to `Sequence[C;assicalDataStoreReader]`.

Related: quantumlib#887, quantumlib#3231 (open question @95-martin-orion whether this closes those or not)

This PR contains a `ClassicalDataStoreReader` and `ClassicalDataStoreBase` parent "interface" for the `ClassicalDataStore` class as well. This will allow us to swap in different representations that may have different performance characteristics. See quantumlib#3808 for an example use case. This could be done by adding an optional `ClassicalDataStore` factory method argument to the `SimulatorBase` initializer, or separately to sampler classes.

(Note this is an alternative to quantumlib#4778 for supporting qudits in sympy classical control expressions, as discussed here: https://github.com/quantumlib/Cirq/pull/4778/files#r774816995. The other PR was simpler and less invasive, but a bit hacky. I felt even though bigger, this seemed like the better approach and especially fits better with our future direction, and closed the other one).

**Breaking Changes**:
1. The abstract method `SimulatorBase._create_partial_act_on_args` argument `log_of_measurement_results: Dict` has been changed to `classical_data: ClassicalData`. Any third-party simulators that inherit `SimulatorBase` will need to update their implementation accordingly.
2. The abstract base class `ActOnArgs.__init__` argument `log_of_measurement_results: Dict` is now copied before use. For users that depend on the pass-by-reference semantics (this should be rare), they can use the new `classical_data: ClassicalData` argument instead, which is pass-by-reference.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/qudits area/result complexity/medium introduces/modifies 3-5 concepts, takes max up to a month for an advanced contributor good for learning For beginners in QC, this will help picking up some knowledge. Bit harder than "good first issues" kind/feature-request Describes new functionality skill-level/advanced One or more of the areas need a solid understanding. 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

8 participants