Improve reverse_bits to support registerless bits #7423
Conversation
Pull Request Test Coverage Report for Build 2339124462
💛 - Coveralls |
There was a problem hiding this comment.
Thank you for looking at this!
I've left some comments in line - I think on Terra we're still trying to transition from an old way of looking at registers (they used to be the most important thing) to a new model where bits are the most important, and registers are just a reference to a collection of bits. In particular, I made a comment about how I think this could be much easier just by constructing the circuit with the bit order reversed first, and then just remaking the registers later and adding them.
qiskit/circuit/quantumcircuit.py
Outdated
| (new_cargs, cargs, self._clbit_indices), | ||
| ]: | ||
| for arg in args: | ||
| _, regs = indices[arg] |
There was a problem hiding this comment.
This line could be registers = indices[arg].registers - in general with namedtuple it's better to use named access where possible.
qiskit/circuit/quantumcircuit.py
Outdated
| for bits, indices in [ | ||
| (self.qubits, self._qubit_indices), | ||
| (self.clbits, self._clbit_indices), | ||
| ]: | ||
| i = len(bits) - 1 | ||
| while i >= 0: | ||
| bit = bits[i] | ||
| _, regs = indices[bit] | ||
| if regs: | ||
| reg, local_idx = regs[0] | ||
| circ.add_register(reg) | ||
| i -= reg.size | ||
| else: | ||
| circ.add_bits([bit]) | ||
| i -= 1 |
There was a problem hiding this comment.
We also need to handle cases where registers overlap each other, i.e. the same bit is in multiple registers. I think the cleanest solution to this whole function will be (completely untested, so I could be wrong on the logic):
- create a circuit with no registers, and the same bit instances in reverse order:
out = QuantumCircuit( reversed(self.qubits), reversed(self.clbits), name=self.name, global_phase=self.global_phase, )
- replay all the data from the old circuit into the new circuit without changing the args; because we reversed the bits, and the args all contain references to the bit instances, they'll automatically apply to the correct bits
- go through each register in the old circuit, and make a new register that references the correct bits in the new circuit
I think that's the correct logic, and it makes the correct separation between Bit and Register.
There was a problem hiding this comment.
Thanks for the suggestion. I followed these steps in the commit d0d8cc8.
qiskit/circuit/quantumcircuit.py
Outdated
| if regs: | ||
| reg, local_idx = regs[0] | ||
| arg = reg[reg.size - 1 - local_idx] | ||
| new_args.append(arg) |
There was a problem hiding this comment.
I'm not sure why it's necessary to look in a register at this point?
There was a problem hiding this comment.
I was not aware of the overlap case. This part has been modified.
| def test_reverse_bits_with_registerless_bits(self): | ||
| """Test reversing order of registerless bits.""" | ||
| q0 = Qubit() | ||
| q1 = Qubit() | ||
| c0 = Clbit() | ||
| c1 = Clbit() | ||
| qc = QuantumCircuit([q0, q1], [c0, c1]) | ||
| qc.h(0) | ||
| qc.cx(0, 1) | ||
| qc.x(0).c_if(1, True) | ||
| qc.measure(0, 0) | ||
|
|
||
| expected = QuantumCircuit([c1, c0], [q1, q0]) | ||
| expected.h(1) | ||
| expected.cx(1, 0) | ||
| expected.x(1).c_if(0, True) | ||
| expected.measure(1, 1) | ||
|
|
||
| self.assertEqual(qc.reverse_bits(), expected) |
There was a problem hiding this comment.
This is a good test. Please could you also add some tests where there's a mix of registers and registerless bits, and some where there are some registers that overlap with each other?
There was a problem hiding this comment.
I added three additional test cases. They succeeded with the new logic. The downside may be that the way of constructing the expect circuit is not very straightforward, especially in the cases of overlapping bits.
Also add more test cases.
|
@jakelishman I have read your comments, including the one in #7415. I followed your steps and now can support both registerless qubits and converting little-endian to big-endian. Hope it works! |
javabster
added
the
help wanted
jakelishman
added
Changelog: Bugfix
* Improve reverse_bits to support registerless bits * Change the algorithm of reverse_bits * Reimplement reverse_bits with reversed bits Also add more test cases. * Simplify bit lookups Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Summary
This PR fixes #7415. It modifies
QuantumCircuit.reverse_bitsto support circuits that contain registerless qubit and classical bits.Details and comments
I assume the semantics of
reverse_bitsmethod is to reverse the order of registers but keep the bit order within a register unchanged. This assumption is made based on a test case of the function:https://github.com/Qiskit/qiskit-terra/blob/6742941a62bf1a12c854f028dab15489118b6c15/test/python/circuit/test_circuit_operations.py#L878-L896
To emphasize these semantics, I modified the example circuit diagram in the docstring.
For registerless bits, I regard them as single-bit anonymous registers. Therefore, they are reversed. A test case is added to demonstrate this case. It would fail without the modification.
close #7415