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

FIX: Redefine slope of RampWaveform #644

Merged
merged 4 commits into from
Feb 12, 2024
Merged

FIX: Redefine slope of RampWaveform #644

merged 4 commits into from
Feb 12, 2024

Conversation

a-corni
Copy link
Collaborator

@a-corni a-corni commented Feb 12, 2024

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

@a-corni a-corni requested a review from HGSilveri February 12, 2024 11:37
@a-corni a-corni self-assigned this Feb 12, 2024
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 agree with this correction, I just have a suggestion on the docstring

@@ -533,7 +533,7 @@ class RampWaveform(Waveform):
Args:
duration: The waveform duration (in ns).
start: The initial value (in rad/µs).
stop: The final value (in rad/µs).
stop: The final value at duration - 1 (in rad/µs).
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about

Suggested change
stop: The final value at duration - 1 (in rad/µs).
stop: The value (in rad/µs) at the final sample.

And also change start accordingly.

@a-corni a-corni changed the title Redefine slope FIX: Redefine slope of RampWaveform Feb 12, 2024
start: The initial value (in rad/µs).
stop: The final value at duration - 1 (in rad/µs).
start: The value (in rad/µs) at the initial sample.
stop: The value (in rad/µs) at the initial sample.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small confusion here

Suggested change
stop: The value (in rad/µs) at the initial sample.
stop: The value (in rad/µs) at the final sample.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow that was silly sorry about that 😅

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

@sgrava
Copy link

sgrava commented Feb 12, 2024

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

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

a-corni commented Feb 12, 2024

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)

@a-corni a-corni merged commit 20e6765 into develop Feb 12, 2024
6 of 7 checks passed
@HGSilveri HGSilveri deleted the fix_slope branch April 15, 2024 09:52
@HGSilveri HGSilveri mentioned this pull request Apr 29, 2024
HGSilveri added a commit that referenced this pull request Apr 29, 2024
Main changes:

21a47f3 Remove Register.rotate() (#642)
20e6765 FIX: Redefine slope of RampWaveform (#644)
c2d5b6c Enabling definition of multiple noise channels and noise channels in XY (#647)
bcb78cc Enable digital simulation (#652)
0f6e3dd Improve access to output modulation durations (#663)
188d21d Remove deprecated noise arguments (#674)
f303138 Adding relaxation noise channel (#675)
716b86b Centralize all backend imports from a single pulser.backends module (#678)
96a8c34 Add hyperfine dephasing rate to NoiseModel (#680)
4981ca6 Add optional default noise models to devices (#676)
c695373 Rectangular lattice register and layout (#665)
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.

Confusion over the definition of RampWaveform
3 participants