pasqal-io / Pulser

Library for pulse-level/analog control of neutral atom devices. Emulator with QuTiP.
Apache License 2.0
182 stars 65 forks source link

Add leakage noise in NoiseModel #714

Closed a-corni closed 4 months ago

a-corni commented 4 months ago

Add the option to include leakage in NoiseModel. It's currently done by modifying the parameter with_leakage in NoiseModel. with_leakage is a boolean. It is associated with the noise type "leakage". For backward compatibility reason, with_leakage is an option in the schema of noise_model. I have also added it with_leakage in the default values (if someone uses noise_types). However, I am not sure if the behaviour is correct:

HGSilveri commented 4 months ago

Thanks for this @a-corni ! I was wondering, isn't there some sort of rate for leakage? EDIT: Nevermind, I just remembered that it has to be given by the user through the effective noise parameters

a-corni commented 4 months ago

Thanks for this @a-corni ! ~I was wondering, isn't there some sort of rate for leakage?~ EDIT: Nevermind, I just remembered that it has to be given by the user through the effective noise parameters

Indeed ! I would like to first implement it for effective noise operator, and then have a look at implementing it with dephasing and depolarizing noise. For dephasing, we might want to have a error_dephasing_rate (like the hyperfine_dephasing_rate), but let's see this when it will be needed :)

One thing I am not sure of is the way to go from this PR:

HGSilveri commented 4 months ago

One thing I am not sure of is the way to go from this PR:

* should I merge it ? But then users can only define effective_noise_opers of shape (2, 2) and won't perform simulations with noise. Should I then add a NotImplementedError in Hamiltonian/QutipEmulator saying that simulation with leakage is not implemented yet ?

* should I change this PR to also incorporate the implementation of leakage in pulser-simulation ? Should I make a separate PR starting from these changes ?
  I think the best is going with NotImplementedError in Hamiltonian for the moment... What is your opinion on this ?

Let's go with the NotImplementedError

a-corni commented 4 months ago

I only have a few nits, so it's pretty much good for me. One thing though: wouldn't it perhaps make more sense to merge #715 first?

Thanks for the review ! I have addressed the nits. As you want, we can definitely merge #715 before merging this PR 😄

HGSilveri commented 4 months ago

I only have a few nits, so it's pretty much good for me. One thing though: wouldn't it perhaps make more sense to merge #715 first?

Thanks for the review ! I have addressed the nits. As you want, we can definitely merge #715 before merging this PR 😄

I thought that there were some TODOs here that could be done already, that's why I suggested it

a-corni commented 4 months ago

I only have a few nits, so it's pretty much good for me. One thing though: wouldn't it perhaps make more sense to merge #715 first?

Thanks for the review ! I have addressed the nits. As you want, we can definitely merge #715 before merging this PR 😄

I thought that there were some TODOs here that could be done already, that's why I suggested it

That's True, I will let you merge your PR, I will delete the TODOs in this PR afterwards.

HGSilveri commented 4 months ago

Thanks for this review of the schema. I have followed your suggestions. Should I keep 'with_leakage' property in the repr as well ?

I thought about it and I'm torn actually. It's obviously redundant but if someone sets with_leakage=True I imagine they might expect to see it appear in the repr. I guess leaving it is the safer option, at least it won't cause any confusion

a-corni commented 4 months ago

Thanks for this review of the schema. I have followed your suggestions. Should I keep 'with_leakage' property in the repr as well ?

I thought about it and I'm torn actually. It's obviously redundant but if someone sets with_leakage=True I imagine they might expect to see it appear in the repr. I guess leaving it is the safer option, at least it won't cause any confusion

That's also my feeling, let's go with this :)