Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add leakage noise in NoiseModel #714

Merged
merged 13 commits into from
Jul 29, 2024
Merged

Add leakage noise in NoiseModel #714

merged 13 commits into from
Jul 29, 2024

Conversation

a-corni
Copy link
Collaborator

@a-corni a-corni commented Jul 25, 2024

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:

  • by default, the parameter with_leakage is False in the init, and to have leakage the user should set with_leakage to True.
  • however, if the user asks for leakage in noise_types, then I guess he doesn't want to also set with_leakage to True, so the default value of with_leakage in _LEGACY_DEFAULTS is True.
  • In SimConfig, the user only defines leakage in the noises, and not via with_leakage, that is only a property instead of an argument.

@a-corni a-corni requested a review from HGSilveri July 25, 2024 08:40
@HGSilveri
Copy link
Collaborator

HGSilveri commented Jul 25, 2024

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

pulser-core/pulser/noise_model.py Outdated Show resolved Hide resolved
pulser-core/pulser/noise_model.py Outdated Show resolved Hide resolved
pulser-core/pulser/noise_model.py Outdated Show resolved Hide resolved
pulser-core/pulser/noise_model.py Outdated Show resolved Hide resolved
pulser-core/pulser/noise_model.py Outdated Show resolved Hide resolved
pulser-core/pulser/noise_model.py Outdated Show resolved Hide resolved
pulser-core/pulser/noise_model.py Outdated Show resolved Hide resolved
pulser-core/pulser/noise_model.py Show resolved Hide resolved
pulser-simulation/pulser_simulation/simconfig.py Outdated Show resolved Hide resolved
pulser-core/pulser/noise_model.py Outdated Show resolved Hide resolved
@a-corni
Copy link
Collaborator Author

a-corni commented Jul 26, 2024

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:

  • 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 ?

@HGSilveri
Copy link
Collaborator

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

pulser-core/pulser/noise_model.py Outdated Show resolved Hide resolved
pulser-core/pulser/noise_model.py Outdated Show resolved Hide resolved
pulser-core/pulser/noise_model.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

tests/test_noise_model.py Outdated Show resolved Hide resolved
tests/test_noise_model.py Outdated Show resolved Hide resolved
tests/test_noise_model.py Outdated Show resolved Hide resolved
@a-corni
Copy link
Collaborator Author

a-corni commented Jul 29, 2024

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
Copy link
Collaborator

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

Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@a-corni
Copy link
Collaborator Author

a-corni commented Jul 29, 2024

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.

Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@HGSilveri HGSilveri self-requested a review July 29, 2024 14:21
Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I forgot to check the schema

Copy link
Collaborator Author

@a-corni a-corni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@HGSilveri
Copy link
Collaborator

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

Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I think we're good!

@a-corni
Copy link
Collaborator Author

a-corni commented Jul 29, 2024

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 :)

@a-corni a-corni merged commit 82bedf5 into develop Jul 29, 2024
7 checks passed
@a-corni a-corni deleted the add_leakage branch July 29, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants