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

BlockCollapser fails for ClassicalRegister conditions #10496

Closed
chriseclectic opened this issue Jul 25, 2023 · 2 comments · Fixed by #10505
Closed

BlockCollapser fails for ClassicalRegister conditions #10496

chriseclectic opened this issue Jul 25, 2023 · 2 comments · Fixed by #10505
Assignees
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@chriseclectic
Copy link
Member

chriseclectic commented Jul 25, 2023

Environment

  • Qiskit Terra version: 0.24 and 0.25rc1
  • Python version: All
  • Operating system: All

What is happening?

The BlockCollapser class fails for blocks containing multiple nodes with full ClassicalRegister conditions on the same registers.

How can we reproduce the issue?

Minimal example

from qiskit import QuantumCircuit
from qiskit.converters import circuit_to_dag
from qiskit.dagcircuit.collect_blocks import BlockCollector, BlockCollapser

qc = QuantumCircuit(1, 2)
qc.x(0).c_if(qc.cregs[0], 0)
qc.y(0).c_if(qc.cregs[0], 1)

dag = circuit_to_dag(qc)
blocks = BlockCollector(dag).collect_all_matching_blocks(
    lambda _: True, split_blocks=False, min_block_size=1
)
dag = BlockCollapser(dag).collapse_to_operation(blocks, lambda circ : circ.to_instruction())
---------------------------------------------------------------------------
CircuitError                              Traceback (most recent call last)
Cell In [5], line 14
      8 dag = circuit_to_dag(qc)
     10 blocks = BlockCollector(dag).collect_all_matching_blocks(
     11     lambda _: True, split_blocks=False, min_block_size=1
     12 )
---> 14 dag = BlockCollapser(dag).collapse_to_operation(blocks, lambda circ : circ.to_instruction())

File ~/mambaforge/envs/pec/lib/python3.9/site-packages/qiskit/dagcircuit/collect_blocks.py:368, in BlockCollapser.collapse_to_operation(self, blocks, collapse_fn)
    366 # Add classical registers used in conditions over registers
    367 for reg in cur_clregs:
--> 368     qc.add_register(reg)
    370 # Construct a quantum circuit from the nodes in the block, remapping the qubits.
    371 wire_pos_map = {qb: ix for ix, qb in enumerate(sorted_qubits)}

File ~/mambaforge/envs/pec/lib/python3.9/site-packages/qiskit/circuit/quantumcircuit.py:1425, in QuantumCircuit.add_register(self, *regs)
   1421 for register in regs:
   1422     if isinstance(register, Register) and any(
   1423         register.name == reg.name for reg in self.qregs + self.cregs
   1424     ):
-> 1425         raise CircuitError('register name "%s" already exists' % register.name)
   1427     if isinstance(register, AncillaRegister):
   1428         for bit in register:

CircuitError: 'register name "c" already exists'

What should happen?

No error.

Any suggestions?

The collapse_to_operation method is incorrectly collecting the full classical register from all conditional nodes in a block as a list, when it should be a set to avoid duplication.

@chriseclectic chriseclectic added the bug Something isn't working label Jul 25, 2023
@chriseclectic chriseclectic added this to the 0.25.0 milestone Jul 25, 2023
@jakelishman
Copy link
Member

Thanks for the report, and yeah, that does appear to be the direct reason.

@jakelishman jakelishman added the good first issue Good for newcomers label Jul 25, 2023
@cjh1
Copy link
Contributor

cjh1 commented Jul 25, 2023

I would like to take a crack at this one, if someone can assign it to me.

cjh1 added a commit to cjh1/qiskit-terra that referenced this issue Jul 26, 2023
This adds a test case that adds multiple conditions on the same
register. The test case is fixed by using a set to collect the
classical registers in collapse_to_operation.
@mtreinish mtreinish modified the milestones: 0.25.0, 0.25.1 Jul 27, 2023
@mtreinish mtreinish modified the milestones: 0.25.1, 0.25.2 Aug 17, 2023
@1ucian0 1ucian0 modified the milestones: 0.25.2, 0.25.3 Sep 28, 2023
@mtreinish mtreinish modified the milestones: 0.25.3, 0.45.1 Oct 24, 2023
@jakelishman jakelishman modified the milestones: 0.45.1, 0.45.0 Oct 31, 2023
github-merge-queue bot pushed a commit that referenced this issue Oct 31, 2023
* Collect classical registers using a set (#10496)

This adds a test case that adds multiple conditions on the same
register. The test case is fixed by using a set to collect the
classical registers in collapse_to_operation.

* Add release note

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
mergify bot pushed a commit that referenced this issue Oct 31, 2023
* Collect classical registers using a set (#10496)

This adds a test case that adds multiple conditions on the same
register. The test case is fixed by using a set to collect the
classical registers in collapse_to_operation.

* Add release note

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
(cherry picked from commit b47c28b)
github-merge-queue bot pushed a commit that referenced this issue Nov 1, 2023
* Collect classical registers using a set (#10496)

This adds a test case that adds multiple conditions on the same
register. The test case is fixed by using a set to collect the
classical registers in collapse_to_operation.

* Add release note

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
(cherry picked from commit b47c28b)

Co-authored-by: Chris Harris <cjh@lbl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants