Added static attribute num_params to all standard gates

[javabster]

Summary

• Adds static attributes to all standard gate classes:
  • name lower case string name of the gate
  • num_params number of angle params the class requires

Details and comments

This PR originally was created for #6185 but due to the complexity of the solution it has now been split into 2x PRs. This one covers the creation of the static attributes, and the twolocal changes have been moved to #7100

[jakelishman]

Not at all sure it's the cause of the test failure, but you've got name = "mcx" on three different gates - C3XGate, C4XGate and MCXGate.

[javabster]

Not at all sure it's the cause of the test failure, but you've got name = "mcx" on three different gates - C3XGate, C4XGate and MCXGate.

Yep thanks I'm aware, they're mostly placeholders for now :)

[javabster]

why test_gate_qasm_with_ctrl_state is currently failing:

• test calls .qasm() function. The if statement in quantumcircuit.py line 1560 onwards relies on calling instruction.name
• instruction.name in main branch calls the name property in controlledgate.py, which checks if the gate has open controls and returns "ch_o0" in this test case
• instruction.name in my branch returns the static attribute name for the CHGate which is just "ch", causing the test to fail

I'm not familiar with qasm at all so not sure what the best approach is in this case, maybe rename the property in controlledgate.py to something else other than name? Also only one test failed because of this naming conflict but is it possible this clash between the property and the static attribute could cause issues elsewhere too?

Any advice would be greatly appreciated 😊

[jakelishman]

I think this runs a little deeper than just QASM, that's just where the problem became visible. I think we decided that Instruction.name should uniquely identify a type of instruction, so we need to know if |0><0|.I + |1><1|.H and |0><0|.H + |1><1|.I are similar to different parametrisations. As long as our data model thinks about gates the same way that qasm2 does, then those are different gates (i.e. they're not just different parametrisations of the same gate), so they need to have different values for Instruction.name. This is assumed in various transpiler passes, where the basis gates are given as strings, which must match Instruction.name. If hardware implements ch, it doesn't necessarily implement ch(ctrl_state=0) natively, so they need to have different names in order for the basis translator to do its job right. I guess that means that subclasses of ControlledGate simply wouldn't be able to have a class-level static variable for the time being.

If the model moves to more of a QASM-3-like one, with significant classical control flow, this won't be a problem any more, because we'd be swapping to the ctrl@ and invctrl@ modifiers, keeping the same base gate name (which I guess would be h, even for CHGate).

[javabster]

Ok that makes sense, except CHGates (and other controlled gates) were supported by TwoLocal before so for the purposes of this PR we still need a way to get the lowercase string name. A few options I can think of:

1. go back to having static attributes called gate_name for all standard library gates
2. change static attribute name to gate_name just for the control gates, and have _get_gate_instance() check for both name and gate_name
3. use the original method in TwoLocal for instantiating the control gates with a hardcoded valid_layers dictionary (this option maybe defeats the purpose of this PR somewhat)

Any other suggestions welcome 😄

[jakelishman]

Any instance of ControlledGate represents two separate gates, so they really just can't have one static name that identifies the class. I guess that comment in TwoLocal is just slightly mistaken with the way gates work currently. We could discuss whether it's a design decision we really want to have in the Terra meeting for gates, but as it stands, I just don't think it's possible to completely make this change you're trying for TwoLocal :(. (I didn't notice this earlier, or I would have said then, too, sorry.)

That said, I still think it's worthwhile doing this kind of thing where we can, even if it doesn't entirely work out for TwoLocal here. Parameters that can be static probably should be, and taking it further, if we were to remove label as an option for most gates, then we could make all of the non-parametrised gates entirely static, and make them just singleton objects, which would be more time- and memory-efficient when we're dealing with large numbers of them.

[Cryoris]

I agree with Jake's last comment, this would maybe fix the issue that large circuits consume a large amount of memory. I think the TwoLocal use case is quite specific and shouldn't be the one scenario we should adjust the gates to 🙂

So to me Abby's option 3 together with Jake's comment make sense: keep TwoLocal as is but still add the static attributes. If in the end we can still simplify TwoLocal that's great but if not that's ok too.

Is the name getter of the controlled gates the only thing keeping us from making the name a static attribute? If yes, could we not just make the qasm method add the suffix for open controls instead of doing it in the name method? Like

# in Instruction
name = "ch"  # static attribut

# in circuit's qasm() method
name = instruction.name
if isinstance(instruction, ControlledGate) and instruction._open_ctrl:
   name += # open control suffix like _o2

This assumes that the name with the open control suffix, like ch_o1 is only used in the qasm method.

[javabster]

Ok I've reverted the TwoLocal changes (moved them into another PR #7100) so this one can just focus on adding the static attributes to the standard gates.

Are there any other static attributes we think it would be useful to have other than name and num_params?

[kdk]

Are there any other static attributes we think it would be useful to have other than name and num_params?

num_qubits (and num_clbits) would likely also be useful. We would need to decide on a convention for variadic gates/instructions. (e.g. MSGate or Barrier can accept any number of qargs, so do they have num_qubits=-1 or None, or something else?)

[Cryoris]

Maybe we can reduce this PR to the number of qubits/clbits and the number of parameters. In TwoLocal we could also just support passing the gate via type or we write a small snippet there that tries to map "<name>" to qiskit.circuit.library.<NAME>Gate which should cover all relevant cases, I think.

[jakelishman]

Looking back on this PR (it's been quite a while now) - adding a static name (also for proposed num_qubits and num_clbits) doesn't actually affect the memory usage because the initialiser of Instruction attempts to assign to self.name, self.num_qubits and self.num_clbits as well. The "static" (stored on the class object) attribute doesn't get modified by these assignments, but instead a new entry in the instance's __dict__ is added. Subsequent attribute accesses will then pull from the instance dict rather than the class object following the normal v-lookup rules of Python.

For the future: with enhanced classical processing, and with the intent of reducing memory usage, I've proposed in Qiskit/RFCs#26 a move towards encoding a "type signature" on gates and circuits, which would also cover all the things suggested here, I think (except name).

[Cryoris]

Maybe we can revive this for the 0.24 release 🙂

• regarding the name: if the suggestion to change the name only in .qasm() doesn't work we could also just drop the name for now. But maybe the suggestion is worth a try 🙂
• num_qubits we could also add, that does sound useful. As @kdk suggested, I think None as default for gates with variable width (like Barrier) sounds good 👍🏻
• would it be good to add a base class, e.g. StandardGate, in which all types have the static attributes we add here?

[coveralls]

Pull Request Test Coverage Report for Build 4445539346

• 58 of 58 (100.0%) changed or added relevant lines in 27 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 85.392%

---

Totals
Change from base Build 4443897467:	0.01%
Covered Lines:	68062
Relevant Lines:	79705

---

💛 - Coveralls

[javabster]

So we came across an issue with both name and num_qubits where a whole bunch of tests broke because certain parts of the code started pulling the static attribute values even when the class had been instantiated and had new values for those attributes. After discussion with @Cryoris we decided to remove the name and num_qubits aspect of this work and just focus on num_params for now. We also discussed the possibility of creating a StandardGate base class at some point that could prevent these kinds of issues.

[jakelishman]

I think I've lost the original reason for this change now on this PR, sorry.

What's the intended use-case for this static num_params field? It's not a field on Instruction or Gate, so in general, if we've got an Instruction we can't know if it's got this field or not, so it's not really safe to try and use it.

(fwiw: I checked the files and agree that all the num_params fields are correct)

[javabster]

@jakelishman the original purpose for adding the num_params was support the original issue to improve TwoLocal (i think we outlined the approach here) and then throughout the process it was mentioned that it would be a good thing to have generally (maybe @Cryoris has a more specific example)

[jakelishman]

Looking back to that underlying TwoLocal issue: since that issue was opened, we now have qiskit.circuit.library.standard_gates.get_standard_gate_name_mapping, which knows how to construct all the "standard" Qiskit-defined gates, including how many parameters they need. If the intent is to fix that, then that's probably going to be the cleanest strategy. There's no way defined to reliably construct a user-defined gate, so we're always going to be limited to Qiskit-defined ones, and that function is a tested source of truth for them.

I'm not sure that adding a static num_params field is going to be as useful as we might be hoping, because num_params isn't a part of the Instruction or Operation interfaces at all, so no code expecting one of those (or a class object for one of them) can rely on this property existing at the moment. I'm not super keen to add it to the base classes right now, either, because I'm hoping to add a more complete "signature" mechanism for operations to specify both the number and types of the "run-time" arguments they expect (though admittedly, I have been hoping to do that for a long time).

[Cryoris]

Oh I wasn't aware that mapping exists now, that would solve the use case here. So after a long back and forth it seems we found a good solution for the original use case 😬

[jakelishman]

It didn't at the time you opened the issue, or even when Abby opened this PR - it's relatively recent (maybe Terra 0.22, I think?).

[Cryoris]

Yeah, git blame says it's from 6 months ago and this PR is from 2021 😅

[jakelishman]

Given the above comments, the difficulties we had with all the edge cases involved, and that the underlying use-case for this got solved another way, I'll close this PR as staled now. Thanks for the work, and feel free to re-open if there's more to discuss.