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

Rework the NoiseModel interface #697

Closed
HGSilveri opened this issue Jun 20, 2024 · 2 comments · Fixed by #710
Closed

Rework the NoiseModel interface #697

HGSilveri opened this issue Jun 20, 2024 · 2 comments · Fixed by #710
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@HGSilveri
Copy link
Collaborator

There are few issues with the existing NoiseModel interface:

  1. Cumbersome UX: When activating a set of noises, they have to be given to the noise_types argument and the corresponding parameters have to be modified from their defaults. For example, to include only a 10% rate of false positives, one needs to define NoiseModel(noise_types=("SPAM",), p_false_pos=0.1, p_false_neg=0., state_prep_error=0.). Preferably, one should only need to define NoiseModel(p_false_pos=0.1).

  2. Error-prone: In fact, the "preferred way" shown above seems to be something users are intuitively doing already and it currently results in no noise being actually included, which is admittedly misleading.

  3. Arbitrary/misleading default values: The default values for every parameter where inherited from the SimConfig class, which was created in a time where providing reasonable defaults was needed. Now that we can associate default noise models to devices, keeping some universal default values makes much less sense. Furthermore, having non-zero default values is an issue in itself, since it includes noise through parameters the user never explicitly defined.

  4. Confusing __repr__(): As a consequence of having all these default values, the NoiseModel.__repr__() (autogenerated because NoiseModel is a dataclass) is a confusing mess of default values.

Proposed changes:

  1. Deprecate the default values: We can't just change them because it will break existing code (or worse, silently change the results). However, we can deprecate them and mark them for removal in v1.

  2. Not requiring the explicit definition of noise_types: Instead of relying on the user "activating" the noises they want, we can move to an interface where they just specify the parameters that should be taken into account and the corresponding noise type is internally "activated". For now this will work well because each noise parameter is associated to a unique noise type. However, I can see at some point a parameter like temperature being associated to more than one noise type (eg. Doppler noise and thermal motion of the atoms). In this case, having the desired noise type(s) explicitly defined would be needed for finer control, so I don't think we can completely move away from explicit noise type definition. Nonetheless, we can still make it an optional parameter that is generally not required.

  3. Define a custom __repr__(): We can make it so only the active noises and associated parameters appear.

@HGSilveri HGSilveri added the enhancement New feature or request label Jun 20, 2024
@HGSilveri HGSilveri added this to the Pulser v1 milestone Jun 20, 2024
@lvignoli
Copy link
Collaborator

lvignoli commented Jun 20, 2024

Thanks a lot for this neat description!
I support all your claims. To me it makes sense to toggle a noise channel when a non-zero value for a parameter bound to it is specified by the user.

As you described in change 2., just from a runtime perspective, users still need a way to toggle noise channels afterwards if a parameter is bound to several. Having two methods to call successively seems the most natural, like set_noise_parameters and set_noise_channels or so. Otherwise, we could have a single method but with non-scalar objects for parameters bound to several channels. For example, temperature would be a struct with a float decorated with flags to turn on/off the required channel. It sounds a bit intricate.

An alternate possibility could be to not bind any parameter to more than 1 noise channel. So you would have a temperature_doppler and a temperature_thermal_motion for example. That looks a bit funny and clunky, but it's straightforward. And maybe this can be backed physically: could each of them have a different effective temperature?.

@HGSilveri
Copy link
Collaborator Author

As you described in change 2., just from a runtime perspective, users still need a way to toggle noise channels afterwards if a parameter is bound to several. Having two methods to call successively seems the most natural, like set_noise_parameters and set_noise_channels or so. Otherwise, we could have a single method but with non-scalar objects for parameters bound to several channels. For example, temperature would be a struct with a float decorated with flags to turn on/off the required channel. It sounds a bit intricate.

An alternate possibility could be to not bind any parameter to more than 1 noise channel. So you would have a temperature_doppler and a temperature_thermal_motion for example. That looks a bit funny and clunky, but it's straightforward. And maybe this can be backed physically: could each of them have a different effective temperature?.

Thank you for your insights! I think we agree that there is no perfect solution for this case.
NoiseModel is supposed to be a simple dataclass, so I would like to avoid methods that change its state after initialization. I think the struct with a decorated float would hurt ease of use a bit too much, so I would avoid that too if possible.
I don't dislike the idea of restricting a parameter to no more than one noise channel, it has the benefit of being the most straightforward. I think we can assess what makes most sense when the time comes, at least we know we have options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants