Skip to content

Fix BooleanHamiltonian.with_qubits() to work with not only NamedQubits #4396

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

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

tonybruguier
Copy link
Contributor

@tonybruguier tonybruguier commented Aug 9, 2021

The current code at head does not implement BooleanHamiltonian.with_qubits() correctly. It assumes that the qubits are named and then maps them to the variable name.

This is restrictive, does not follow the other classes' with_qubits(). It is also incorrect because it silently accepts a list of qubits with missing entries (e.g. there is a variable name 'a' but we don't provide a qubit named 'a'), and the error is thrown only when decompose_once() is called.

Instead, here the code accepts a list of qubits and replaces them in the map. It requires that there are as many qubits provided as there were when building the object. Also, it relies on the order of the qubit keys when building the BooleanHamiltonian object, but that is currently a restriction of the API.

Fixes #4390.

@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Aug 9, 2021
@tonybruguier tonybruguier marked this pull request as ready for review August 9, 2021 19:31
@tonybruguier tonybruguier requested review from cduck, vtomole and a team as code owners August 9, 2021 19:31
@tonybruguier tonybruguier requested a review from maffoo August 9, 2021 19:31
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - can you add a descriptive description for the PR? That becomes the commit message, we should be pedantic about it. Also add "Fixes #4390." as the last line. Thanks!

@tonybruguier tonybruguier changed the title Attempt to fix issue #4390 BooleanHamiltonian no longer incorrect accepts only named qubits Aug 9, 2021
@tonybruguier tonybruguier changed the title BooleanHamiltonian no longer incorrect accepts only named qubits BooleanHamiltonian.with_qubits() no longer incorrect accepts only NamedQubit, but rather follows the other classes definition of with_qubits() Aug 9, 2021
@tonybruguier
Copy link
Contributor Author

@balopat
Done

@tonybruguier tonybruguier changed the title BooleanHamiltonian.with_qubits() no longer incorrect accepts only NamedQubit, but rather follows the other classes definition of with_qubits() BooleanHamiltonian.with_qubits() no longer incorrectly accepts only NamedQubit, but rather follows the other classes definition of with_qubits() Aug 9, 2021
@balopat balopat changed the title BooleanHamiltonian.with_qubits() no longer incorrectly accepts only NamedQubit, but rather follows the other classes definition of with_qubits() Fix BooleanHamiltonian.with_qubits() to work with not only NamedQubits Aug 10, 2021
@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Aug 10, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Aug 10, 2021
@CirqBot CirqBot merged commit 9d695c5 into quantumlib:master Aug 10, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Aug 10, 2021
@tonybruguier tonybruguier deleted the bool_qubit branch August 10, 2021 03:33
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
quantumlib#4396)

The current code at head does not implement BooleanHamiltonian.with_qubits() correctly. It assumes that the qubits are named and then maps them to the variable name.

This is restrictive, does not follow the other classes' with_qubits(). It is also incorrect because it silently accepts a list of qubits with missing entries (e.g. there is a variable name 'a' but we don't provide a qubit named 'a'), and the error is thrown only when decompose_once() is called.

Instead, here the code accepts a list of qubits and replaces them in the map. It requires that there are as many qubits provided as there were when building the object. Also, it relies on the order of the qubit keys when building the BooleanHamiltonian object, but that is currently a restriction of the API.

Fixes quantumlib#4390.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
quantumlib#4396)

The current code at head does not implement BooleanHamiltonian.with_qubits() correctly. It assumes that the qubits are named and then maps them to the variable name.

This is restrictive, does not follow the other classes' with_qubits(). It is also incorrect because it silently accepts a list of qubits with missing entries (e.g. there is a variable name 'a' but we don't provide a qubit named 'a'), and the error is thrown only when decompose_once() is called.

Instead, here the code accepts a list of qubits and replaces them in the map. It requires that there are as many qubits provided as there were when building the object. Also, it relies on the order of the qubit keys when building the BooleanHamiltonian object, but that is currently a restriction of the API.

Fixes quantumlib#4390.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BooleanHamiltonian.with_qubits is broken
3 participants