-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Retain ordering information in conversion. #4384
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
will actually play with this and such later, but just for posterity while I'm thinking about it, the specific repro case that this is addressing (that I brought to Jon's attention on a different channel) is reconstructing multiple measurement keys into a histogram, i.e. import cirq
import numpy as np
import interface as interface
import sympy
qubit=cirq.LineQubit.range(2)
theta=np.pi
circuit=cirq.Circuit()
circuit.append(interface.h(qubit[0]))
circuit.append(interface.h(qubit[1]))
for i in range(5):
circuit.append(interface.cnot(qubit[0],qubit[1]))
circuit.append(cirq.rz(sympy.Symbol('phi')).on(qubit[1]))
circuit.append(interface.cnot(qubit[0],qubit[1]))
circuit.append(interface.h(qubit[0]))
circuit.append(interface.h(qubit[1]))
resolved=cirq.resolve_parameters(circuit,{"phi":1.5/18*np.pi})
for i in range(len(resolved.all_qubits())):
resolved.append(cirq.measure(cirq.LineQubit(i)))
job=service.create_job(circuit=resolved,repetitions=1000,target="qpu")
job.results().to_cirq_result().multi_measurement_histogram(keys=["0","1"]) v.s. a simple job.results().counts() or creating a histogram from a measurement key that included both qubits to begin with: resolved.append(cirq.measure(qubit[0], qubit[1], key="both"))
job=service.create_job(circuit=resolved,repetitions=1000,target="qpu")
job.results().to_cirq_result().histogram(key="both") these should theoretically all be the same, but because we don't preserve measurement correlation when building via counts(key) and simple list comprehension, we lose this information, which causes the first example to produce different composite counts than the latter two. Yes, in both cases we're faking the real measurement list (i.e. the IonQ API doesn't actually give us an ordered list of individual measurements) but at least this way we're faking it in a way that behaves better with Cirq's assumptions about that list |
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.
A small test case would be very useful for analyzing this change :)
cirq-ionq/cirq_ionq/results.py
Outdated
@@ -39,8 +40,40 @@ def repetitions(self) -> int: | |||
"""Returns the number of times the circuit was run.""" | |||
return self._repetitions | |||
|
|||
def ordered_results(self, key: Optional[str] = None) -> List[List[int]]: | |||
""" Returns a list of arbitrarily but consistently orderd results. |
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.
nits
typo: ordered
also extra space
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
cirq-ionq/cirq_ionq/results.py
Outdated
@@ -26,7 +26,8 @@ class QPUResult: | |||
def __init__( | |||
self, counts: Dict[int, int], num_qubits: int, measurement_dict: Dict[str, Sequence[int]] | |||
): | |||
self._counts = counts | |||
# Guaranteed iter ordering is required for conversion to Cirq results. | |||
self._counts = collections.OrderedDict(counts) |
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.
If feels strange to me that this is taking a dict, which has no ordering constraints, and then converting it to an ordered dict. I think if the ordering is necessary, shouldn't the counts parameter have that contract?
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.
note that python 3.6 dicts are insertion ordered in cpython, but this is an implementation detail. in 3.7 this becomes the standard. So since Cirq still supports 3.6 we need to use OrderedDict.
I think you want to move the OrderedDict up to the Job results method.
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.
Changed this to be more explicit that we're sorting here by bitvector to get ordering. WDYT?
Here is a test case that shows the problem
|
test case added, I took a slightly different tack than you did, lmk what you think :) |
Updated some type and formatting that the tests didn't like. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
Looks like you have format errors. run This should also fix lint. Looks like you also hit a flakey test, investigating. |
Aye aye! No worries re: radar, thanks for circling back :) |
@dabacon friendly ping (been a while since I've pinged the likes of you :) !) |
adding the automerge tag back for Jon now that the format errors are fixed and tests are passing and such because he doesn't have the perms to do it himself. Feel free to revert if this feels out of line |
Previously, when you requested a cirq result with measurement A, and B, we would return a Result object that contained the correct counts for A and B, but which eliminated any correlation information we had (by throwing the results order out in count()). This changes so that count() is derived from another method, ordered_results, which takes a key and returns the list of list of bit-wise results in a consistent way from key to key.
Previously, when you requested a cirq result with measurement A, and B, we would return a Result object that contained the correct counts for A and B, but which eliminated any correlation information we had (by throwing the results order out in count()). This changes so that count() is derived from another method, ordered_results, which takes a key and returns the list of list of bit-wise results in a consistent way from key to key.
Previously, when you requested a cirq result with measurement A, and B, we would return a Result object that contained the correct counts for A and B, but which eliminated any correlation information we had (by throwing the results order out in count()). This changes so that count() is derived from another method, ordered_results, which takes a key and returns the list of list of bit-wise results in a consistent way from key to key.