Fix to classical registers in to_qasm

[BrunoLiegiBastonLiegi]

Small fix to how the classical registers and measurements are converted to qasm.
This should close #1283

Checklist:

• Reviewers confirm new code works as expected.
• Tests are passing.
• Coverage does not decrease.
• Documentation is updated.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.84%. Comparing base (8fd5657) to head (78c8056).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1284      +/-   ##
==========================================
- Coverage   99.85%   99.84%   -0.01%     
==========================================
  Files          73       73              
  Lines       10690    10689       -1     
==========================================
- Hits        10674    10672       -2     
- Misses         16       17       +1     

Flag	Coverage Δ
unittests	99.84% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stavros11]

I think the issue with #1283 is in from_qasm. The fix implemented here fixes the issue at the QASM level, however the Qibo circuit created by from_qasm is still a bit problematic because it contains two different registers with the same name:

from qibo import Circuit, gates

qasm = 'OPENQASM 2.0;\ninclude "qelib1.inc";\nqreg q[2];\ncreg m0[2];\ny q[0];\nx q[1];\nmeasure q[0] -> m0[0];\nmeasure q[1] -> m0[1];'

qibo_cirq = Circuit.from_qasm(qasm)

for gate in qibo_cirq.queue:
    if isinstance(gate, gates.M):
        print(gate, gate.qubits, gate.register_name)

prints

<qibo.gates.measurements.M object at 0x78a057949f10> (0,) m0
<qibo.gates.measurements.M object at 0x78a078565610> (1,) m0

instead of

<qibo.gates.measurements.M object at 0x78a057949f10> (0, 1) m0

that I would expect.

This has undesired effects if the circuit is simulated:

result = qibo_cirq(nshots=10000)

then

In [3]: result.frequencies(registers=False)
Out[3]: Counter({'11': 10000})

In [4]: result.frequencies(registers=True)
Out[4]: {'m0': Counter({'1': 10000})}  # one of the registers is ignored because you cannot have the same key twice

Allowing different measurements with the same register_name in the circuit is kind of debatable, personally I would raise an error if the same register_name is used more than once, but others may disagree. However, either way I think that the given qasm should be translated to a single measurement gates.M(0, 1, register_name="m0") instead of two as it does currently.

[BrunoLiegiBastonLiegi]

Ah I see. When I rewrote the from_qasm I explicitely modified the behaviour of the registers because I thought that doing something like:

c.add(gates.M(0, register="m0"))
c.add(gates.M(1, register="m0"))

should not raise an error. In my opinion, it is perfectly fine to, in some sense, 'append' measurements to a register. Therefore, now I see three different options:

• Fix frequencies, and most likely samples as well, to handle this possibility
• Implement some kind of merge_registers routine that merges measurements associated to the same register, but this is problematic for the MeasurementResult I believe, because the results:

m0 = c.add(gates.M(0, register="m0"))
m1 = c.add(gates.M(1, register="m0"))

are not gonna be associated to the merged measurement anymore.

• Drop the support for this and revert the changes

Probably the last one is indeed the easiest and less bug prone option...

[stavros11]

Ah I see. When I rewrote the from_qasm I explicitely modified the behaviour of the registers because I thought that doing something like:

c.add(gates.M(0, register="m0"))
c.add(gates.M(1, register="m0"))

should not raise an error. In my opinion, it is perfectly fine to, in some sense, 'append' measurements to a register. Therefore, now I see three different options:

Thanks for explaining, that makes sense. I agree that it is fine to provide this possibility, but of course as it is implemented now there is this side-effect when asking for frequencies(registers=True) and samples(registers=True) where only the latest measurement is included in the returned results and the rest are ignored.

• Implement some kind of merge_registers routine that merges measurements associated to the same register, but this is problematic for the MeasurementResult I believe, because the results:

m0 = c.add(gates.M(0, register="m0"))
m1 = c.add(gates.M(1, register="m0"))

are not gonna be associated to the merged measurement anymore.

I have not thought in detail, but it seems possible to make m1 point to the same object as m0, but this may require some manipulations inside Circuit.add. The general idea is that Circuit (I believe) has access to all registers/measurement gates, so when you do c.add(gates.M(1, register="m0")) you can check if "m0" register already exists, add a new qubit to it and return the existing result object instead of creating a new one. Of course results need to be mutable for that, but I believe they already are.

• Drop the support for this and revert the changes

Probably the last one is indeed the easiest and less bug prone option...

I agree with this. The easiest solution is to just raise an error and not support reusing register names and only fix the qasm issue in this PR. We could also open an issue in case we wish to implement this feature later. From the user side doing

c.add(gates.M(0, register="m0"))
c.add(gates.M(1, register="m0"))

is not much different than

c.add(gates.M(0, 1, register="m0"))

but there may be an application that I am missing that the former could be useful.

[BrunoLiegiBastonLiegi]

The only case that comes to my mind is whether you would like to measure different qubits at different times but in the same classical register. Thus, for instance something like this:

c.add(gates.H(0))
c.add(gates.M(0, register="m0"))
c.add(gates.H(1))
c.add(gates.CX(1,0))
c.add(gates.M(1, register="m0"))

but this is very niche, and probably there's no real reason to have both the measurements in the same register anyway.

[MatteoRobbiati]

Thanks! It works for me.