qiboteam / qibo

A framework for quantum computing
https://qibo.science
Apache License 2.0
288 stars 59 forks source link

Composite noise model interferes with `circuit.measurements.register_name` #1211

Open rahula06 opened 7 months ago

rahula06 commented 7 months ago

Describe the bug Applying the composite noise model to a circuit leads to the circuit.measurements.register_name attribute to be "None", leading to issues in functions that need this, such as circuit.to_qasm()

To Reproduce

from qibo.models import Circuit
from qibo import gates
from qibo.noise import NoiseModel

c = Circuit(4)

c.add(gates.RX(0,2.5,True))
c.add(gates.M(1))

print(c.draw())

print("Start of printing measurements!")
for i in c.measurements:
    print(i)
    print(i.register_name)
print("End of printing measurements!")

par = {"t1" : [0.1],
        "t2" : [0.1],
        "gate_time" : (0.1, 0.1),
        "excited_population" : 0.1,
        "depolarizing_error" : (0.1, 0.1),
        "bitflips_error" : ([0.1 for _ in range(4)], [0.1 for _ in range(4)]),
        "idle_qubits" : 0
        }

noise = NoiseModel()
noise.composite(par)

c = noise.apply(c)

print("Start of printing measurements!")
for i in c.measurements:
    print(i)
    print(i.register_name)
print("End of printing measurements!")

Expected Behaviour

The above code will show different values for the register name in the print statement, although applying a noise model should have no effect on the measurement registers. Tested on Qibo 0.2.4.

@jykhoo1987 @shangtai

### Tasks
- [ ] https://github.com/qiboteam/qibo/issues/1213
- [ ] https://github.com/qiboteam/qibo/issues/1214
renatomello commented 7 months ago

Looking at this, it seems that the issue is a little bit deeper.

For instance, the noisy_circuit function in noise_model.py does the exact same thing as NoiseModel.apply already does, but specializing for three specific noise channels (ThermalRelaxationChannel, DepolarizingChannel, and classical bitflip). Moreover, the NoiseModel.composite that @rahula06 is using calls CompositeNoiseModel from noise_model.py, which in fact is just a way to call the aforementioned noisy_circuit function that, again, does the same thing as NoiseModel.apply already does.

@AlejandroSopena since apparently you wrote this, do you know why this is the case?

As a minor comment, the function hellinger_shot_error in noise_model.py should be moved to quantum_info.utils

EDIT: @rahula06 if you use the standard NoiseModel.apply method, there is no error.

from qibo import Circuit, gates
from qibo.noise import NoiseModel, DepolarizingError

circuit = Circuit(4)
circuit.add(gates.RX(0, 2.5))
circuit.add(gates.M(1))

print("Start of printing measurements!")
for k in circuit.measurements:
    print(k)
    print(k.register_name)
print("End of printing measurements!")

noise = NoiseModel()
noise.add(DepolarizingError(0.1))

circuit_noisy = noise.apply(circuit)

print(circuit_noisy.draw())

print("Start of printing measurements!")
for k in circuit_noisy.measurements:
    print(k)
    print(k.register_name)
print("End of printing measurements!")
rahula06 commented 7 months ago

Thanks @renatomello. Yep, the standard NoiseModel object does not mess with the measurements. As for the two noisy_circuit functions, I see that the CompositeNoiseModel is a completely separate class to the NoiseModel class, perhaps because it doesn't apply a noise channel in the same way. Rather, it applies a collection of them in a certain manner and with certain parameters to simulate hardware. A logical way to do it could be for the CompositeNoiseModel to just initialize a NoiseModel object with the right channels inside it, although I'm not too sure of the details and there could be some challenges :)

EDIT: Seeing that you have opened an issue to get rid of the CompositeNoiseModel altogether, I think there might be utility in keeping it, since it provides the user with an easy one-step way to add realistic noise to the circuit.

renatomello commented 7 months ago

As for the two noisy_circuit functions, I see that the CompositeNoiseModel is a completely separate class to the NoiseModel class, perhaps because it doesn't apply a noise channel in the same way.

My point is that it shouldn't.

Rather, it applies a collection of them in a certain manner and with certain parameters to simulate hardware.

That's already covered by NoiseModel.

A logical way to do it could be for the CompositeNoiseModel to just initialize a NoiseModel object with the right channels inside it, although I'm not too sure of the details and there could be some challenges :)

There are only two things that I see different there: 1) fixed noise channels that can be initiated by passing parameters in a dictionary; 2) some optimization step (I'm not sure why but I'm sure it's useful).

The first point: if the noise model with fixed channels is important (e.g. I know reported noise models from superconducting chips like IBM always report DepolarizingChannel followed by ThermalRelaxationChannel and with some classical bitflip in the end), then it should be a class that inherits NoiseModel and fixes the channels. This way the apply method does not need to be recreated from scratch (to avoid redundancy and the possibility of introducing bugs like we're seeing here)

The second point: this optimization step should be a class in qibo.optimizers, inheriting either NoiseModel or the new class defined in the point above.

EDIT: Seeing that you have opened an issue to get rid of the CompositeNoiseModel altogether, I think there might be utility in keeping it, since it provides the user with an easy one-step way to add realistic noise to the circuit.

So I guess the TL;DR is that it seems like everything in noise_model.py is either a redundancy or in the wrong place.

rahula06 commented 7 months ago

Yep, it seems that there's nothing in there that couldn't be built using standard NoiseModel, I just meant that the functionality of CompositeNoiseModel is useful. A class that inherits NoiseModel makes sense.

AlejandroSopena commented 7 months ago

We implemented this noise model for the master's thesis of a student of @scarrazza. In this sense we wanted to create a new class with his model. It is based on the IBM model with depolarizing and thermal noise. The difference is that we added thermal channels to model idle qubits. I'm not sure if this can be taken into account using the NoiseModel class, but I can take a look.

The optimization method allows determining these parameters (T1, T2, and depolarizing parameter) from measurements given a circuit. We expect they are close to the values reported experimentally (taking into account the simplicity of the model). I agree that this part can be moved to qibo.optimizers.

renatomello commented 7 months ago

The difference is that we added thermal channels to model idle qubits. I'm not sure if this can be taken into account using the NoiseModel class, but I can take a look.

Thermal relaxation after either gates.I or gates.Align

rahula06 commented 7 months ago

A related issue: a noisy circuit after applying CompositeNoiseModel does not accept statevector input (needs a density matrix), throwing an error saying it cannot cast into the right dimensions. The same is not true for the standard NoiseModel. This is a little bit like beating a dead horse, the fix is the same, but just want to highlight that there might be more inconsistencies with the CompositeNoiseModel that we don't know about.

renatomello commented 7 months ago

A related issue: a noisy circuit after applying CompositeNoiseModel does not accept statevector input (needs a density matrix), throwing an error saying it cannot cast into the right dimensions. The same is not true for the standard NoiseModel. This is a little bit like beating a dead horse, the fix is the same, but just want to highlight that there might be more inconsistencies with the CompositeNoiseModel that we don't know about.

Thank you. The best to solution is to avoid using anything from noise_model.py until we rewrite a composite noise model that properly inherits NoiseModel. In the meantime, you can use your own composite noise model via NoiseModel

scarrazza commented 7 months ago

@renatomello for the next release, what is the status here?

renatomello commented 7 months ago

There's still one part of noise_model.py that needs to be moved to qibo.optimizers. I'd close this issue and create a new one just for that snippet

On Fri, 1 Mar 2024 at 14:10, Stefano Carrazza @.***> wrote:

@renatomello https://github.com/renatomello for the next release, what is the status here?

— Reply to this email directly, view it on GitHub https://github.com/qiboteam/qibo/issues/1211#issuecomment-1972898714, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABH5QVTHGJOPQSP2KCPE3TTYWBHYVAVCNFSM6AAAAABDF7E4NKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZSHA4TQNZRGQ . You are receiving this because you were mentioned.Message ID: @.***>

scarrazza commented 3 months ago

@renatomello, please let us know when this issue can be closed.

renatomello commented 3 months ago

@renatomello, please let us know when this issue can be closed.

This wasn't closed yet because the optimization step inside noise_model.py was not transferred anywhere else like the other parts of that noise class were. If @AlejandroSopena is OK with it, I can open a PR deleting that file and we close this issue. Otherwise, that optimization step needs to be placed somewhere before closing it.