Make qasm output valid gate names to fix #7052

[epelaaez]

Summary

Identifies invalid QASM gate names and converts them into valid names to fix #7052.

Details and comments

Right now, the same snippet of code is being run on the main for loop in the qasm function and in the _add_sub_instruction_to_existing_composite_circuits function that is called inside qasm for nested gate calls. Since the functionality is the same in both parts, should I create a function that does this so it can be called where needed and avoid having the same piece of code twice?

Such function would look like _ensure_name_validity(instruction: Instruction) -> Instruction, where the same instruction is returned if the name is valid and if it is invalid, a copy of the instruction with the modified valid name will be returned. If its best to create such function, should I add this to the end of the file with the other functions that start with _?

[epelaaez]

Hi @jakelishman, thanks for the feedback!

I've implemented the function _qasm_escape_gate_name(name: str) -> str and use it where needed. Additionaly, I noticed that the unitary object sometimes created the qasm string for a unitary with its own method, and that is then used in the general quantum circuit qasm() method. Therefore, I also added the needed fix there.

[jakelishman]

This looks very close to me, thanks for the effort! I had one incredibly minor point about naming (that you wouldn't really have known about), and one about another test case I thought of. I think it's very close to being done, though, so thanks for this!

[jakelishman]

This is a very good test, thanks! One other case that might be good to test as well: can we add two custom gates that have different invalid names, but their escaped forms would be the same? In other words, something like:

base = QuantumCircuit(1)

clash1 = QuantumCircuit(1, name="invalid??")
clash1.x(0)
base.append(clash1, [0])

clash2 = QuantumCircuit(1, name="invalid[]")
clash2.z(0)
base.append(clash2, [0])

and ensure that the de-duplication code correctly runs (so at least one of the names becomes "invalid__<id number>")? As written, both of these will escape their names to invalid__, so we need to make sure the de-duplication triggers correctly.

[epelaaez]

Thanks for this idea!

The de-duplication code is ran correctly, but I think it's hard to check for that. The gates first go through the check that ensures they are valid identifiers, and, if they are not, a copy of the instruction is made with the valid name. Then, this copy is the one that goes through the de-duplication code, which appends the id at the end of the name.

For other tests, the id could be accessed since it is the one from the original instruction. But, in this case, the id appended is the one from the copy created by the code that ensures the name is valid. Therefore, doing the following in the test won't return the correct expected qasm.

expected_qasm = f"""OPENQASM 2.0;
include "qelib1.inc";
gate invalid__ q0 {{ x q0; }}
gate invalid___{id(clash2)} q0 {{ z q0; }}
qreg q[1];
invalid__ q[0];
invalid___{id(clash2)} q[0];\n"""

Is there a way to check that an id is appended to the end of the second instruction name without specifying that id (so, using regex)? Or what should be the approach here?

[jakelishman]

We could cheat a little - we don't necessarily care how the names are de-deduplicated, just that somehow or another, they are. Given we control the input, we can limit the expected output to things we can "parse" with regular expressions: perhaps something along the lines of:

names = set()
for match in re.findall(r"gate (\S+)", base.qasm()):
    gate_name = match.group(1)
    self.assertTrue(VALID_QASM2_IDENTIFIER.fullmatch(gate_name))
    names.add(gate_name)
self.assertEqual(len(names), ...)

In words:

1. find each instance of the string "gate <gate_name> ..."
2. extract the gate name from it
3. check it's a valid name
4. add it to a set (automatic uniqueness check)
5. assert the length of the set is what we expect

That tests the behaviour we want (all names are valid, and there are no duplicates), without testing specifics of the implementation (how exactly the deduplication checker works). What I've just sketched there might not be 100% perfect, but I think the idea should work?

[epelaaez]

I've added this test. Thanks for the code! It was very helpful. I just found out doing gate_name = match.group(1) wasn't neccesary since what is returned is already the string we are looking for because the regex pattern only has one group. Anyways, it is working now!

[jakelishman]

Ah yeah, nice spot thanks. I messed up the distinction between re.findall and re.finditer - finditer returns match objects, findall returns the results of capture groups.

[jakelishman]

Looks good to me, thanks for sticking through it!

[jakelishman]

Ah yeah, nice spot thanks. I messed up the distinction between re.findall and re.finditer - finditer returns match objects, findall returns the results of capture groups.

[jakelishman]

Oh no, wait, I'm sorry I forgot: could you add a bug fix release note (reno new --edit fix-qasm-invalid-names) mentioning that we've fixed the invalid names?

needs reno

[epelaaez]

Added release note, thanks for all the help @jakelishman!

[jakelishman]

Perfect, thanks!