pasqal-io / Pulser

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

Rework the NoiseModel interface #697

Closed HGSilveri closed 2 months ago

HGSilveri commented 3 months ago

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.

lvignoli commented 3 months ago

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 commented 3 months ago

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.