search

pasqal-io / Pulser

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

FIX: Redefine slope of RampWaveform #644

Closed a-corni closed 4 months ago

a-corni commented 4 months ago

Fixing the definition of slope, for it to match with our current definition of RampWaveform, as associating [start, start + (start - end)/(duration - 1), ..., end] to [0, 1, ..., duration - 1] Trying to fix the confusion in the documentation. @sgrava Closes #645

sgrava commented 4 months ago

Hi @a-corni and @HGSilveri Thanks for having a look into this!

Do you think there will be some room to discuss changing the sampling behaviour of the ramp in the future? I totally agree that it might be painful, but I think it makes more sense for the slope of the ramp to be exactly (stop-start)/duration.

HGSilveri commented 4 months ago

Do you think there will be some room to discuss changing the sampling behaviour of the ramp in the future? I totally agree that it might be painful, but I think it makes more sense for the slope of the ramp to be exactly (stop-start)/duration.

Hey @sgrava ! There's always room to discuss :) I agree with you that this definition of slope is counterintuitive, but I think it's more important that the sampling behaviour is in line with all the other waveforms. If the slope property is cause for confusion or mistakes, I would rather advocate for its deprecation because it seems we can't fully reconcile it with the way we define waveforms.

Nonethless, it's also important to note that these differences are most likely negligible at the timescales our devices operate. We shouldn't stress too much about them, in my opinion.

a-corni commented 4 months ago

Do you think there will be some room to discuss changing the sampling behaviour of the ramp in the future? I totally agree that it might be painful, but I think it makes more sense for the slope of the ramp to be exactly (stop-start)/duration.

Hey @sgrava ! There's always room to discuss :) I agree with you that this definition of slope is counterintuitive, but I think it's more important that the sampling behaviour is in line with all the other waveforms. If the slope property is cause for confusion or mistakes, I would rather advocate for its deprecation because it seems we can't fully reconcile it with the way we define waveforms.

Nonethless, it's also important to note that these differences are most likely negligible at the timescales our devices operate. We shouldn't stress too much about them, in my opinion.

I also think it's important to have consistence. Changing the behaviour of one Waveform is tricky, since a waveform is usually defined with others, and letting the option to the user to extend the duration of RampWaveforms by 1 would yield an error for the other waveforms. As an example, at the moment you have to define a duration of 101 to divide your ramp by 100, but then the other waveforms need to be defined by the same duration of 101. However, letting the possibility for the user to define a RampWaveform with coefficient 100 would impose on the others to define the other waveforms with a duration of 101 - and it might be harder to get where the error is coming from and for a beginner to fix it. Moreover, as Henrique is saying, at the timescale and values we are operating these are very small variation (1e-3 rad/mu s over 1ns for a pulse of duration 100ns) such that we are close to the simulation error I believe (1e-6 rad variation in the previous example)