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

arbitrary order in circuit drawers #8063

Closed
1ucian0 opened this issue May 15, 2022 · 17 comments · Fixed by #8173
Closed

arbitrary order in circuit drawers #8063

1ucian0 opened this issue May 15, 2022 · 17 comments · Fixed by #8173
Assignees
Labels
help wanted community contributions welcome. For filters like http://github-help-wanted.com/ type: feature request New feature or request unitaryhack Issues/PR participating (now or in the past) in the UnitaryHack event see https://unitaryhack.dev/

Comments

@1ucian0
Copy link
Member

1ucian0 commented May 15, 2022

What should we add?

Circuit drawers currently have reverse_bits, as an option to modify the order in which wires are represented. @ajavadia suggests a generalized version of that feature: wire_order. This parameter might define the order of the wires draw(..., wire_order=[3,0,2,1]) and can eventually replace reverse_bits with draw(..., wire_order='reverse'.

@1ucian0 1ucian0 added type: feature request New feature or request help wanted community contributions welcome. For filters like http://github-help-wanted.com/ labels May 15, 2022
@enavarro51
Copy link
Contributor

enavarro51 commented May 16, 2022

In addition to the circuit drawers reverse_bits there's a QuantumCircuit.reverse_bits which doesn't currently work for registerless bits. Issue #7415 and open PR #7423 were opened to address this and there are discussions there. First question is whether there is a use case for QuantumCircuit.reverse_bits independent of the drawers. Right now it's possible to do QuantumCircuit.reverse_bits and add reverse_bits in the drawers, which probably does not always give you the original order back.

If we want to keep QuantumCircuit.reverse_bits it could make sense to simply call this right before calling get_layered_instructions in visualization/circuit_visualization.py. In that case all the current internal code for handling the reverse_bits option in the 3 drawers could be removed.

Since wire_order is just a generalization of reverse_bits, wire_order could be added to QuantumCircuit and the same process could be done in the drawers. Alternatively if the use case for QuantumCircuit.reverse_bits independent of the drawers is weak, we could focus all this on wire_order at the drawer level.

@enavarro51
Copy link
Contributor

enavarro51 commented May 17, 2022

I previously had worked on a version of the circuit drawers that used QuantumCircuit.reverse_bits prior to get_layered_instructions and removed the reverse_bits code from the drawers as discussed above. I was waiting on #7423 to merge which it did today. This code produces this in the mpl drawer for this somewhat convoluted circuit,

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

bits = [Qubit(), Qubit(), Qubit(), Clbit(), Clbit()]
bits1 = [Clbit()]
cr = ClassicalRegister(2)
crx = ClassicalRegister(4)
qc = QuantumCircuit(bits, cr, [Clbit()], crx, bits1)
qc.x(0).c_if(bits[3], 1)
qc.x(0).c_if(cr[0], True)
qc.x(1).c_if(bits[4], 1)
qc.x(1)
qc.x(1)
qc.measure(0, cr)
qc.x(2).c_if(bits1[0], 0)
qc.draw('mpl', cregbundle=True, reverse_bits=True, scale=0.7)

image
If you add qc = qc.reverse_bits() before the draw command, it returns the original circuit,
image
So the question is, is this the expected behavior? If so, I can proceed with a PR for this. I think additional work will be required for the latex and text drawers, but should be doable.

Following this, a generalization for wire_order should be fairly straightforward.

@1ucian0 1ucian0 added the unitaryhack Issues/PR participating (now or in the past) in the UnitaryHack event see https://unitaryhack.dev/ label May 18, 2022
@ajavadia
Copy link
Member

I think QuantumCircuit.reverse_bits() and QuantumCircuit.draw(reverse_bits=True) are slightly different, no? Because the former actually changes the circuit (an H that was done on q_0 is now done on q_N), whereas the latter just draws differently (q0 at the top or qN at the top).

BTW I'm fine adding an option to QuantumCircuit to permute wires. Perhaps permute_bits is a better name. So we can have QuantumCircuit.permute_bits([3,1,0,2]), or QuantumCircuit.draw(permute_bits=[3,1,0,2]) --- the first one changes the circuit data. The second one changes the drawing.

@jakelishman
Copy link
Member

The drawers don't necessarily need to be concerned with reordering things, though - you can always do qc.permute_bits([...]).draw().

@enavarro51
Copy link
Contributor

So is there any real reason to just reverse the bits in the drawing, as @ajavadia notes above in a way that's different from the way it's done in QuantumCircuit.reverse_bits()? It seems the whole original reason for doing it was because qiskit ordered bits differently from what was done some other places, and this is solved with the QuantumCircuit version.

As @jakelishman notes, it's simple enough to do both qc.permute_bits([...]).draw() and qc.reverse_bits().draw(). We could do what I suggested above for reverse_bits in the drawer as a backwards compatibility.

@ajavadia
Copy link
Member

ajavadia commented May 21, 2022

But what I'm saying is that QuantumCircuit.reverse_bits() is not just a change in visualization, it actually changes the circuit. So I think we need both the circuit option and the visualization option.

In [5]: qc.draw()
Out[5]:
     ┌───┐
q_0: ┤ H ├
     └───┘
q_1: ─────

q_2: ─────


In [6]: qc.draw(reverse_bits=True)    # same circuit, just drawn differently (top-to-bottom instead of bottom-to-top)
Out[6]:

q_2: ─────

q_1: ─────
     ┌───┐
q_0: ┤ H ├
     └───┘

In [7]: qc.reverse_bits().draw()   # a different circuit: H is now applied to a different qubit
Out[7]:

q_0: ─────

q_1: ─────
     ┌───┐
q_2: ┤ H ├
     └───┘

and so we need to add a generalization of these in the form of QuantumCiruit.permute_bits([....]) and QuantumCircuit.draw(permute_bits=[...])

@jakelishman
Copy link
Member

jakelishman commented May 23, 2022

I buy Ali's point above. Copying over a comment I had from our internal discussion:

I don’t know how the reversal is handled in the drawers right now, but it probably doesn't need to be complicated code. It should just be the case of carrying around an “order” mapping with you, and you can completely unify the code - the “regular” drawer is just a special case of the “permute” drawer.

@javabster
Copy link
Contributor

My 2 cents:

  • I think we should use different names for the QuantumCiruit.permute_bits([....]) and QuantumCircuit.draw(permute_bits=[...]) as they seem to be doing different things (and i don't think their reverse_bits predecessors should have been named the same thing in the first place tbh)

  • we aren't removing reverse_bits() or reverse_bits= in this issue right? I think it's still useful to keep around, although we should probably emphasise the differing functionality in the documentation somehow

@enavarro51
Copy link
Contributor

Some questions.

  • The permute_bits is defined using integers only? As in [3, 1, 0, 4]. As opposed to using qubits and registers in the way these are passed to QuantumCircuit.
  • The permute_bits is qubits only? If includes clbits, there would need to be 2 permute_bits lists, since qubits and clbits are currently numbered separately. Could combine them, but might get confusing for users. Also would require turning off cregbundle since clbits in a register might not now be contiguous.
  • The len of the permute_bits list must match the len of the qubits list? As in, no partial reordering?
  • As currently envisioned, the QuantumCircuit.permute_bits(...) and draw(permute_bits=... would produce the same visual output?
  • What about reorder_bits for the draw option?

@1ucian0
Copy link
Member Author

1ucian0 commented May 31, 2022

  • I think we should use different names for the QuantumCiruit.permute_bits([....]) and QuantumCircuit.draw(permute_bits=[...]) as they seem to be doing different things (and i don't think their reverse_bits predecessors should have been named the same thing in the first place tbh)

I agree. I suggest QuantumCircuit.draw(wire_order=[...]).

  • we aren't removing reverse_bits() or reverse_bits= in this issue right? I think it's still useful to keep around, although we should probably emphasise the differing functionality in the documentation somehow

We are not. We are defining a generalization of QuantumCircuit.draw(reverse_bits=[...]). Internally, reverse_bits might use this new parameter.

  • The permute_bits is defined using integers only? As in [3, 1, 0, 4]. As opposed to using qubits and registers in the way these are passed to QuantumCircuit.

I would say so. Many of the questions are under the umbrella "how smart this parameter should be?". I lean towards a dumb parameter and let the caller to have that smartness, as the parameter is an advance feature.

  • The permute_bits is qubits only? If includes clbits, there would need to be 2 permute_bits lists, since qubits and clbits are currently numbered separately. Could combine them, but might get confusing for users.

I would combine them. Under my "dumb parameter" principle, it's trivially doable by the smart caller to do [..., qc.num_qubits+<number>].

Also would require turning off cregbundle since clbits in a register might not now be contiguous.

Yes. I agree, cregbundle==False

  • The len of the permute_bits list must match the len of the qubits list? As in, no partial reordering?

"dumb parameter" principle again, the length should match qc.num_qubits+qc.num_clbits.

  • As currently envisioned, the QuantumCircuit.permute_bits(...) and draw(permute_bits=... would produce the same visual output?

I would leave permute_bits out of this issue for now. In principle, yes QuantumCircuit.permute_bits(...).draw() == draw(permute_bits=...)

  • What about reorder_bits for the draw option?

Not sure if I follow this one.

@enavarro51
Copy link
Contributor

All sounds good. For this first PR, we just address the drawers and leave QuantumCircuit.permute_bits for later.

The reorder_bits was just a suggested name for the option. wire_order works fine.

@enavarro51
Copy link
Contributor

A couple of more questions,

If a user uses both reverse_bits and wire_order, do we

  • raise
  • prioritize one or the other
  • do wire_order and then reverse it

If the the wire_order list is not the right length or does not have a unique integer for each bit, do we

  • raise using the same error message for each
  • raise with separate explanations
  • warn and continue without using wire_order

@1ucian0
Copy link
Member Author

1ucian0 commented Jun 6, 2022

If a user uses both reverse_bits and wire_order, do we

I think raise is reasonable. It is probably a user mistake to set both.

If the the wire_order list is not the right length or does not have a unique integer for each bit, do we

raise with separate explanations

@prakharb10
Copy link
Contributor

Hi, is @enavarro51 or someone else working on this?

@enavarro51
Copy link
Contributor

Yeah, I'm currently working on it.

@prakharb10
Copy link
Contributor

Okay, cool. 👍🏻

@1ucian0
Copy link
Member Author

1ucian0 commented Jun 10, 2022

@prakharb10, since this issue is part of the unitary hack initiative, I think it is fine if more than one person works on it in parallel or in teams. So feel free to jump in :) Consider that this particular issue might be a bit advance as a first contribution. There is a good first issue tag that are also part of the unitaryhack initiative if you add the phrase [unitaryhack] in the PR title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted community contributions welcome. For filters like http://github-help-wanted.com/ type: feature request New feature or request unitaryhack Issues/PR participating (now or in the past) in the UnitaryHack event see https://unitaryhack.dev/
Projects
Status: Done
6 participants