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

Check the existence of AncillaQubits before adding #7450

Merged
merged 12 commits into from
Jan 21, 2022

Conversation

yjt98765
Copy link
Contributor

Summary

The bits in an AncillaRegister are appended to QuantumCircuit._ancilla without checking:

https://github.com/Qiskit/qiskit-terra/blob/9cc9225f249d3a895d9f3944175ea7b681d248c0/qiskit/circuit/quantumcircuit.py#L1354-L1355

This caused the issue described in #7445.

Details and comments

I fix this problem by checking the existence of AncillaQubit before the appending, which is similar to the operation regarding QuantumRegister. A test case is added to demonstrate the change. The test failed without the modification.

close #7445

@3yakuya
Copy link
Contributor

3yakuya commented Jan 1, 2022

Also appears to be blocked by #7457

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

LGTM but maybe @kdk could have a look too 🙂

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this, it looks sensible to me. I left one comment about an extra test that would be nice (it'll already pass), and please could you add a small "bug fix" release note?

Comment on lines 952 to 968
def test_add_existing_bits(self):
"""Test add registers whose bits have already been added."""
qc = QuantumCircuit()
for bit_type, reg_type in (
[Qubit, QuantumRegister],
[Clbit, ClassicalRegister],
[AncillaQubit, AncillaRegister],
):
bits = [bit_type() for _ in range(10)]
reg = reg_type(bits=bits)
qc.add_bits(bits)
qc.add_register(reg)

self.assertEqual(qc.num_qubits, 20)
self.assertEqual(qc.num_clbits, 10)
self.assertEqual(qc.num_ancillas, 10)

Copy link
Member

Choose a reason for hiding this comment

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

Please could we add a test that calling add_register with two different AncillaRegisters containing the same bits, without calling add_bits at all, also adds the correct number of bits? With this PR it will succeed, because AncillaRegister derives from QuantumRegister, but it'd be good to ensure that it's always the case - we can't rely on add_bits having been called by the user first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. It has been added with the name test_overlapped_add_register_and_add_register

@jakelishman jakelishman added Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable labels Jan 4, 2022
@jakelishman jakelishman added this to the 0.19.2 milestone Jan 4, 2022
@coveralls
Copy link

coveralls commented Jan 4, 2022

Pull Request Test Coverage Report for Build 1731098913

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0005%) to 83.142%

Totals Coverage Status
Change from base Build 1730828618: 0.0005%
Covered Lines: 51998
Relevant Lines: 62541

💛 - Coveralls

@yjt98765
Copy link
Contributor Author

yjt98765 commented Jan 5, 2022

@jakelishman Sure. The release note had been added.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the fix!

@mergify mergify bot merged commit 84c3985 into Qiskit:main Jan 21, 2022
mergify bot pushed a commit that referenced this pull request Jan 21, 2022
* Check the existance of AncillaQubits before adding

* Add a test case that calls add_register twice

* Add a release note

* Reword release note

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>
(cherry picked from commit 84c3985)
mergify bot added a commit that referenced this pull request Jan 22, 2022
* Check the existance of AncillaQubits before adding

* Add a test case that calls add_register twice

* Add a release note

* Reword release note

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Kevin Krsulich <kevin.krsulich@ibm.com>
(cherry picked from commit 84c3985)

Co-authored-by: Jintao YU <yjt98765@gmail.com>
@yjt98765 yjt98765 deleted the hotfix-7445 branch January 24, 2022 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bottom.QuantumCircuit.tensor(top. inplace=True) doubles ancillas on modified bottom
6 participants