Allow decomposing composite gates by name

[enavarro51]

Summary

Fixes #9136

Details and comments

This fixes a situation where passing the name of a composite gate to QuantumCircuit.decompose would result in the gate not decomposing if there was a label on the gate.

[qiskit-bot]

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

• @Cryoris
• @Qiskit/terra-core
• @ajavadia

[coveralls]

Pull Request Test Coverage Report for Build 3942368030

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.08%) to 84.849%

---

Files with Coverage Reduction	New Missed Lines	%
src/sabre_swap/mod.rs	2	99.54%

Totals
Change from base Build 3942236674:	0.08%
Covered Lines:	65741
Relevant Lines:	77480

---

💛 - Coveralls

[Cryoris]

I left some preliminary comments, but I'm not sure this behavior is what we want. The idea of decomposing by name was to decompose according to what was drawn in the circuit drawer, but with this PR that's not true anymore. For example, with the circuit of the test:

      ┌────────┐               ┌───┐┌─────────────┐
q0_0: ┤0       ├────────────■──┤ H ├┤0            ├
      │        │            │  └───┘│  circuit-93 │
q0_1: ┤1 gate1 ├────────────■───────┤1            ├
      │        │┌────────┐  │       └─────────────┘
q0_2: ┤2       ├┤0       ├──■──────────────────────
      └────────┘│        │  │
q0_3: ──────────┤1 gate2 ├──■──────────────────────
                │        │┌─┴─┐
q0_4: ──────────┤2       ├┤ X ├────────────────────
                └────────┘└───┘

specifying "circuit-*" should only decompose the top-right box, but now it also decomposes gate1 and gate2, to

      ┌───┐               ┌───┐┌───┐
q0_0: ┤ H ├────────────■──┤ H ├┤ X ├
      ├───┤            │  └───┘└───┘
q0_1: ┤ T ├────────────■────────────
      ├───┤┌───┐       │
q0_2: ┤ X ├┤ H ├──■────■────────────
      └───┘└───┘┌─┴─┐  │
q0_3: ──────────┤ X ├──■────────────
      ┌───┐     └───┘┌─┴─┐
q0_4: ┤ X ├──────────┤ X ├──────────
      └───┘          └───┘

but this is not how it was originally designed. If we do want that we can change it, but I just want to make sure that this right now is a breaking change.

In the original design it was intended that XXPlusYYGate was indeed only decomposed by

circuit.decompose("{XX+YY}")

and not by tagging "xx_plus_yy".

[jakelishman]

Julien: I wasn't aware that was the intention from the PR that added it, but even learning it now, it feels a bit odd to me. There's no inherent link between decompose and the drawers, and no link stated in the documentation, or anything like that. decompose is also used by people outside the context of the drawers (such as a quick way to open up some gates into more fundamental definitions), so it doesn't seem like a great idea to tie them too tightly.

The only documentation that exists on using strings as the decomposition is in the Decomposer class (which no-one will see), which says "gates_to_decompose: ... identified by gate label, name or type." To me, that suggests both label and name should work. At the very least, I think we'll need to update the documentation of Decomposer (and QuantumCircuit.decompose) if it's meant to be another way, but I would argue in favour of what Edwin's done here - you can see from the linked issue the current way is already causing confusion for gates that have to define a label for other reasons relating to the drawer.

[enavarro51]

@Cryoris, @jakelishman. I see both sides of this, so just let me know which way to go. I also noticed that gate has been long deprecated in that method, so I'll remove it. I can add some docs to QuantumCircuit.decompose to better explain the behavior you decide on.

[Cryoris]

Yeah I agree that the mix right now is confusing, it seems we were trying to solve a different problem before. But just decomposing if either of the label or name fits still seems imprecise. Would it be a cleaner solution to separately have either (1) gate type and name (both sort of are the type) and (2) labels? That could be done by eg adding "labels_to_decompose" or something similar.

We could also come up with a different solution for checking what gates are made up of in the drawer, but that would probably be very helpful to have (which is why we made decompose do it before 🙂)

[enavarro51]

@Cryoris Adding the 'labels_to_decompose' could be a good solution for the current situation. It eliminates having to decide if 'name' or 'label' has some kind of priority. Only issue is in the short-term if someone is currently using labels in 'gates_to_decompose', how do we handle that. Do a deprecation for it? @jakelishman Let me know if this seems reasonable to you and I can add it in.

In regard to showing gates composed of circuits in the drawers, I'm currently working on showing the control flow circuit clauses in the 'mpl' drawer. It's not a simple problem, but if I get that working, the same structure could be used to show any composite gate as a circuit, perhaps with an option for the user.

[jakelishman]

Technically we should have a deprecation, but this decomposition by string is sufficiently new (and the behaviour sufficiently unexpected, as evidenced by the ticket) that we can say the previous behaviour of gates_to_decompose decomposing labels was a "bug", and that this labels_to_decompose is a new "feature". It's playing a little fast and loose with the deprecation policy, but I think the impact on users is likely to be very minimal, tending towards positive. I'm happy to move ahead with the two separate arguments.

Those changes to the drawer sound great, Edwin, thanks!

[enavarro51]

In thinking (overthinking) this further, I don't believe the labels_to_decompose is the answer. If we look at the original problem, the user was trying to use circuit-* to decompose all the composite gates and only the composite gates. Because composite gates generate a name with a number that can change from one run to the next, there is no way to specify a single composite gate using the name. This is what I think should happen for these entries in gates_to_decompose

• "circuit-*" - decompose all composite gates
• "my_gate_label*" - decompose all gates that match
• "my_gate_label3" - decompose only that gate

Bottom line is the user can only specify a single or subset of all composite gates by using labels. This can be explained in detail in the docs. So leave the behavior as it is in this PR, with some update to the docs.

[jakelishman]

Edwin: this seems to miss the issue that actually caused this PR to be opened, which was that we were surprised that you can't use the gate_name for XXPlusYYGate (xx_plus_yy) to decompose those gates, because for other unrelated drawing reasons, that gate class has to define different label. That's surprising behaviour, and the goal of this is to ensure that gate name strings (which are a first-class citizen of the Qiskit data model) can be used to select gates in classes that accept string selectors, and label (which is largely a drawer-only feature, and one we're not even convinced will stay) doesn't prevent that.

Decomposer and decompose aren't drawer-specific, and never were (for example, see the PR that introduced Decomposer).

[enavarro51]

Ok. Understood that decompose is not drawer-only. The current PR code solves the original problem as well. If a gate name is entered, all gates with that name will be decomposed whether there are labels on them or not. If labels are given, only gates with those labels will be decomposed.

[jakelishman]

Thanks Edwin - I think this now agrees with what we'd decided. I'll merge this now.