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

Add ability to set a classical condition on a list of bits #7653

Merged
merged 102 commits into from
Mar 20, 2022

Conversation

enavarro51
Copy link
Contributor

Summary

Use list of classical bits in conditions

Details and comments

This PR is a continuation of #6761 by @TharrmashasthaPV. That PR created all the underlying code to represent conditions as a list of bits. This PR extends this by modifying the 3 circuit drawers to properly display conditions that are lists. For example,

clbits = [Clbit(), Clbit(), Clbit()]
qr = QuantumRegister(2, "qr")
cr = ClassicalRegister(3, "cr")
circuit = QuantumCircuit(qr, clbits, cr)
circuit.x(0).c_if([cr[0], clbits[1], cr[1], clbits[2]], 11)
circuit.draw("mpl", cregbundle=False)

image
As part of this, if cregbundle is on and at least one of the bits in the list belongs to a register, then cregbundle is turned off with a warning to the user. This allows full display of the details of the condition.

Also now all conditions in the 'mpl' drawer display the hex value at the bottom to be consistent.

if (isinstance(classical, ClassicalRegister) and val >= 2**classical.size) or (
isinstance(classical, list) and val >= 2 ** len(classical)
):
raise CircuitError("condition value should be less than 2 ^ number of bits")
Copy link
Member

Choose a reason for hiding this comment

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

tested in 9a29cfc

Comment on lines +683 to +689
for node in layer:
if self.cregbundle:
self.cregbundle = check_cregbundle(node, self._circuit)
else:
break
if not self.cregbundle:
break
Copy link
Member

@1ucian0 1ucian0 Mar 20, 2022

Choose a reason for hiding this comment

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

These nested loops are a bit hard to follow. I think it is a case of for/else.

@1ucian0 1ucian0 added the Changelog: New Feature Include in the "Added" section of the changelog label Mar 20, 2022
Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

Thanks @enavarro51 ! I added some error-handling tests for completeness. Let's merge it!

@mergify mergify bot merged commit 592b9ac into Qiskit:main Mar 20, 2022
@jakelishman
Copy link
Member

I've been away for quite a while, so never quite got time to comment on this before it merged. Sorry that this is coming a bit out of nowhere now.

This PR has caused relatively serious regressions in several conversion functions, though those are at least overshadowed by other factors over a complete transpilation workload. Having the visualisations prepared to work with conditions on several bits is certainly very useful, but the problem is that this idea of conditioning on a list of bits like this is not how we'll be moving forwards with the more complex conditionals used in dynamic circuits. This PR only adds support for this style of condition to the visualisations, but the chance of it being added to any simulator is very low, and certainly won't be in the 0.20 release.

I think it might be best to roll back this PR, rather than release known regressions into 0.20 for a feature that doesn't have a use yet. The concept of "conditioning on a list of bits" will exist in the future, and I think a large amount of this PR will be useful (especially the visualisation components), but I think it's best not to extend the (bit, value) 2-tuple when it's something that we'll want to be deprecating. Simulator support for conditionals on lists of bits won't be available until the classical type system is in place, so I think it may be better to hold off on this until we're ready to do it a bit more completely.

@jakelishman
Copy link
Member

jakelishman commented Mar 21, 2022

This idea of "conditioning on a list of bits" is actually already supported by Terra, it's just not supported on ad-hoc lists like this PR does. You can currently just add a register to the circuit that contains bits from other registers, and apply the condition on that.

Perhaps the best course of action is to roll back the changes to Instruction that attempt to add list of bits conditionals, and keep the visualisation components. We could update the visualisation components so they work when applied to a register that is not one of the drawn ones, or one that is not defined over contiguous clbits.

The changes to Instruction.condition here are a bit inconsistent, as well. You can condition on a single bit by integer or instance (c_if(0) or c_if(bits[0])), but you can only condition on a list of bit instances:

In [1]: import qiskit
   ...: qc = qiskit.QuantumCircuit(2, 2)
   ...: qc.h(0)
   ...: qc.cx(0, 1)
   ...: qc.measure([0, 1], [0, 1])
   ...: qc.x(0).c_if(qc.clbits, 3)  # ok
   ...: qc.x(0).c_if([0, 1], 3)  # fail
---------------------------------------------------------------------------
CircuitError                              Traceback (most recent call last)
Input In [1], in <cell line: 7>()
      5 qc.measure([0, 1], [0, 1])
      6 qc.x(0).c_if(qc.clbits, 3)  # ok
----> 7 qc.x(0).c_if([0, 1], 3)

File ~/code/qiskit/terra/qiskit/circuit/instructionset.py:195, in InstructionSet.c_if(self, classical, val)
    190     raise CircuitError(
    191         "Cannot pass an index as a condition variable without specifying a requester"
    192         " when creating this InstructionSet."
    193     )
    194 if self._requester is not None:
--> 195     classical = self._requester(classical)
    196 for gate in self.instructions:
    197     gate.c_if(classical, val)

File ~/code/qiskit/terra/qiskit/circuit/quantumcircuit.py:1162, in QuantumCircuit._resolve_classical_resource(self, specifier)
   1160     for bit in specifier:
   1161         if bit not in self._clbit_indices:
-> 1162             raise CircuitError(f"Clbit {specifier} is not present in this circuit.")
   1163         return specifier
   1164 raise CircuitError(f"Unknown classical resource specifier: '{specifier}'.")

CircuitError: 'Clbit [0, 1] is not present in this circuit.'

The correct resolvers in QuantumCircuit aren't updated (Instruction.c_if wasn't the correct source of truth to update here) nor the validation in the new-style control-flow objects, so this also produces a lot of inconsistencies, where list-of-bits conditions can't be used on IfElseOp either directly or in the control-flow builders, and even Instruction.c_if on a list of bit instances (as is the working path in this PR) will fail if called within a builder:

In [3]: with qc.for_loop(range(4)) as _:
   ...:     qc.x(0).c_if(qc.clbits, 3)
   ...:
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Input In [3], in <cell line: 1>()
      1 with qc.for_loop(range(4)) as _:
----> 2     qc.x(0).c_if(qc.clbits, 3)

File ~/code/qiskit/terra/qiskit/circuit/instructionset.py:195, in InstructionSet.c_if(self, classical, val)
    190     raise CircuitError(
    191         "Cannot pass an index as a condition variable without specifying a requester"
    192         " when creating this InstructionSet."
    193     )
    194 if self._requester is not None:
--> 195     classical = self._requester(classical)
    196 for gate in self.instructions:
    197     gate.c_if(classical, val)

File ~/code/qiskit/terra/qiskit/circuit/controlflow/builder.py:314, in ControlFlowBuilderBlock.request_classical_resource(self, specifier)
    312     self.add_bits((resource,))
    313 else:
--> 314     self.add_register(resource)
    315 return resource

File ~/code/qiskit/terra/qiskit/circuit/controlflow/builder.py:361, in ControlFlowBuilderBlock.add_register(self, register)
    354 def add_register(self, register: Register):
    355     """Add a :obj:`.Register` to the set of resources used by this block, ensuring that
    356     all bits contained within are also accounted for.
    357
    358     Args:
    359         register: the register to add to the block.
    360     """
--> 361     if register in self.registers:
    362         # Fast return to avoid iterating through the bits.
    363         return
    364     self.registers.add(register)

TypeError: unhashable type: 'list'

@jakelishman
Copy link
Member

To be clear (and sorry for the third long message): I think we should roll back this PR, but then use the new visualiation logic to make this (currently erroring) code work:

from qiskit.circuit import QuantumCircuit, ClassicalRegister, Clbit, QuantumRegister

clbits = [Clbit() for _ in [None] * 5]
cr1 = ClassicalRegister(bits=clbits)
cr2 = ClassicalRegister(bits=clbits[::2])
qc = QuantumCircuit(QuantumRegister(5), cr1, cr2)
for i in range(5):
    qc.h(i)
qc.measure(range(5), range(5))
qc.x(0).c_if(cr2, 2**3 - 1)
qc.draw()  # ValueError in Layer.set_clbit due to failed register lookup

This shows how "lists of bits" conditions could already be emulated (just not in an ad-hoc manner, and still with no simulator support), and the new visualisation changes I think can be used to make this work in the drawers, which is very useful.

@chriseclectic
Copy link
Member

chriseclectic commented Mar 22, 2022

@jakelishman This PR is also causing test failures on qiskit-experiments, I confirmed reverting this commit fixed the failing tests.

Minimal example related to our test failures:

from qiskit import QuantumCircuit

def teleport_circuit():
    """Teleport qubit 0 to qubit 2"""
    teleport = QuantumCircuit(3, 2)
    teleport.h(1)
    teleport.cx(1, 2)
    teleport.cx(0, 1)
    teleport.h(0)
    teleport.measure(0, 0)
    teleport.measure(1, 1)
    # Conditionals
    creg = teleport.cregs[0]
    teleport.z(2).c_if(creg[0], 1)
    teleport.x(2).c_if(creg[1], 1)
    return teleport

# Try and compose conditional circuit with another one
circ = QuantumCircuit(3, 2)
circ.compose(teleport_circuit(), [0, 1, 2], [0, 1])

Error:

--------------------------------------------------------------------------
TypeError                                Traceback (most recent call last)
<ipython-input-7-5eac41fbe7bc> in <module>
     17 
     18 circ = QuantumCircuit(3, 2)
---> 19 circ.compose(teleport_circuit(), [0, 1, 2], [0, 1])

~/git/qiskit/qiskit-terra/qiskit/circuit/quantumcircuit.py in compose(self, other, qubits, clbits, front, inplace, wrap)
    893                 from qiskit.dagcircuit import DAGCircuit  # pylint: disable=cyclic-import
    894 
--> 895                 n_instr.condition = DAGCircuit._map_condition(edge_map, instr.condition, self.cregs)
    896 
    897             mapped_instrs.append((n_instr, n_qargs, n_cargs))

~/git/qiskit/qiskit-terra/qiskit/dagcircuit/dagcircuit.py in _map_condition(wire_map, condition, target_cregs)
    698             new_cond_val = cond_val
    699             for creg in target_cregs:
--> 700                 if set(creg) == set(new_creg):
    701                     new_cond_val = 0
    702                     for (i, bit) in enumerate(creg):

TypeError: 'Clbit' object is not iterable

chriseclectic added a commit to chriseclectic/qiskit-experiments that referenced this pull request Mar 22, 2022
These two tests were broken by recent qiskit-terra PR #7653: Qiskit/qiskit#7653

This PR is to skip these tests to unblock CI until that commit can be reverted.
jakelishman added a commit to jakelishman/qiskit-terra that referenced this pull request Mar 22, 2022
…iskit#7653)"

This reverts commit 592b9ac.

We intend to keep the visualisation components of this PR, but the
modifications to `Instruction.condition` have caused CI failures in
qiskit-experiments for code that should work.  The same logic in Terra
can currently be represented by adding a new register to the circuit
containing the desired bits (registers are permitted to overlap), and
conditioning on the register.

There is no hardware or simulator support for these cases yet, however
(nor was there before this reversion).  We intend to add a more complete
system of conditional expressions with the move towards more
OpenQASM-3-ish circuits, which will allow conditioning on arbitrary
boolean expressions, not just equality conditions.
@jakelishman
Copy link
Member

jakelishman commented Mar 22, 2022

#7805 will revert the PR as merged (and my sincere apologies, Edwin), though just to be clear for the future: despite the reversion, we still very much want to integrate the changes to the visualisation to support conditions on bits that aren't adjacent for the case of overlapping registers.

For completeness' sake, the reason Experiments' CI failed is because the modifications in this PR to DAGCircuit._map_condition (the lines highlighted in the traceback) don't fully work with single-bit conditions. Experiments is validly using the Terra API.

@mtreinish mtreinish added Changelog: None Do not include in changelog and removed Changelog: New Feature Include in the "Added" section of the changelog labels Mar 22, 2022
mergify bot pushed a commit that referenced this pull request Mar 22, 2022
…7653)" (#7805)

This reverts commit 592b9ac.

We intend to keep the visualisation components of this PR, but the
modifications to `Instruction.condition` have caused CI failures in
qiskit-experiments for code that should work.  The same logic in Terra
can currently be represented by adding a new register to the circuit
containing the desired bits (registers are permitted to overlap), and
conditioning on the register.

There is no hardware or simulator support for these cases yet, however
(nor was there before this reversion).  We intend to add a more complete
system of conditional expressions with the move towards more
OpenQASM-3-ish circuits, which will allow conditioning on arbitrary
boolean expressions, not just equality conditions.
jakelishman added a commit to jakelishman/qiskit-terra that referenced this pull request Mar 22, 2022
Recent work in Qiskit#7653 had introduced a bug where a private method
`DAGCircuit._map_condition` would fail on single-bit conditionals.  This
method is used by `QuantumCircuit.compose`, and our CI should have
caught the issue before merge.  This commit introduces regression tests
to prevent this from happening again.
mergify bot added a commit that referenced this pull request Apr 4, 2022
…se (#7806)

Recent work in #7653 had introduced a bug where a private method
`DAGCircuit._map_condition` would fail on single-bit conditionals.  This
method is used by `QuantumCircuit.compose`, and our CI should have
caught the issue before merge.  This commit introduces regression tests
to prevent this from happening again.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mergify bot pushed a commit that referenced this pull request Apr 4, 2022
…se (#7806)

Recent work in #7653 had introduced a bug where a private method
`DAGCircuit._map_condition` would fail on single-bit conditionals.  This
method is used by `QuantumCircuit.compose`, and our CI should have
caught the issue before merge.  This commit introduces regression tests
to prevent this from happening again.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 528740f)
mergify bot added a commit that referenced this pull request Apr 4, 2022
…se (#7806) (#7878)

Recent work in #7653 had introduced a bug where a private method
`DAGCircuit._map_condition` would fail on single-bit conditionals.  This
method is used by `QuantumCircuit.compose`, and our CI should have
caught the issue before merge.  This commit introduces regression tests
to prevent this from happening again.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 528740f)

Co-authored-by: Jake Lishman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants