-
Notifications
You must be signed in to change notification settings - Fork 34
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
Provide a workaround to #422, Sampler failing when no measurements #426
Conversation
I added a test, @garrison, in |
With this, the current PR will touch one fewer file
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.
Will these circuits with 1 measurement be ignored during reconstruction, since they don't actually have non-trivial observables associated with them?
# measure one qubit as a temporary workaround to | ||
# https://github.com/Qiskit-Extensions/circuit-knitting-toolbox/issues/422 | ||
pauli_indices = cog.pauli_indices | ||
if not pauli_indices: |
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.
What can pauli_indices
be here? Should we just explicitly state what we expect? (e.g. if pauli_indices == []
)
Or we could just assert pauli_indices == []
inside the if? (assuming an empty list is what is returned in I
observable case)
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.
What can pauli_indices be here? Should we just explicitly state what we expect? (e.g. if pauli_indices == [])
It's a sequence of ints. It contains the qubit indices that need to be measured.
The choice of style here is influenced by PEP 8:
For sequences, (strings, lists, tuples), use the fact that empty sequences are false:
# Correct: if not seq: if seq:# Wrong: if len(seq): if not len(seq):
The "nice" thing about doing it this way is it doesn't matter what type the sequence is. For instance, if it were to be a tuple of indices, then it will still behave correctly with the current code, but it would not if we were to compare with []
because () != []
.
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.
nice, thanks for explanation
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.
Wow, this is more profound than I thought. Nice!
Yes |
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.
LGTM 👍
Thanks @IbrahimShehzad and @garrison !
I am leaning toward waiting first until #434 is merged and then updating this branch accordingly. EDIT: Actually, I might have changed my mind already. 😂 |
Co-authored-by: Jim Garrison <garrison@ibm.com>
Co-authored-by: Jim Garrison <garrison@ibm.com>
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.
Everything now looks good to me. Thanks.
) * Provide a workaround to #422, Sampler failing when no measurements * Add test for workaround to Issue #422. * Redo previous commit after running tox. * Fix tox error from previous build. * Fix tox error from last build. * Un-modify the metadata in a notebook With this, the current PR will touch one fewer file * Update test/cutting/test_cutting_roundtrip.py Co-authored-by: Jim Garrison <garrison@ibm.com> * Update test/cutting/test_cutting_roundtrip.py Co-authored-by: Jim Garrison <garrison@ibm.com> * black --------- Co-authored-by: Ibrahim <ibrahim.shehzad@ibm.com> Co-authored-by: Ibrahim Shehzad <75153717+Ibrahim-Shehzad@users.noreply.github.com> (cherry picked from commit f596307)
) (#436) * Provide a workaround to #422, Sampler failing when no measurements * Add test for workaround to Issue #422. * Redo previous commit after running tox. * Fix tox error from previous build. * Fix tox error from last build. * Un-modify the metadata in a notebook With this, the current PR will touch one fewer file * Update test/cutting/test_cutting_roundtrip.py Co-authored-by: Jim Garrison <garrison@ibm.com> * Update test/cutting/test_cutting_roundtrip.py Co-authored-by: Jim Garrison <garrison@ibm.com> * black --------- Co-authored-by: Ibrahim <ibrahim.shehzad@ibm.com> Co-authored-by: Ibrahim Shehzad <75153717+Ibrahim-Shehzad@users.noreply.github.com> (cherry picked from commit f596307) Co-authored-by: Jim Garrison <garrison@ibm.com>
Temporary workaround to #422.
Needs