-
Notifications
You must be signed in to change notification settings - Fork 65
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
Change EmuTN default sampling rate #578
Conversation
Actually this might not be sufficient, EmuTN does not use a sampling rate but rather dt, so I think maybe this interface isn't correct. I've not really reviewed or used it so not sure. EmuTN uses a |
There is an internal conversion from |
The default value for for dt in EmuTN (the |
@awennersteen Can |
The docstring should be updated to reflect the fact that sampling_rate must be quite small for EmuTNBackend (<= 1e-1) to use the backend properly. |
My original intention with That being said, I'm not opposed to this change, I'll defer to your judgement on it. |
Do you mean <= 1e-1? And what do you mean by "properly"? |
Yes, smaller than, I edited the comment. Properly here meaning with acceptable default runtime. |
I understand the original motivation for having some homogeneity in the parameters across the backends. But since different backends serve different purposes, one cannot expect parameters to have the same impact. So it is not a huge loss—to me, at least! |
Ok, gotcha, feel free to add this info to the docstring. |
as Louis says setting sampling_rate so that |
@HGSilveri some test are failing about "good arguments", I am not sure how to fix them. |
You have to change this line: Line 324 in 2315989
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Oh damn, the new matplotlib release has type hints 😨 |
The default sampling rate, inherited from
EmulatorConfig
defaults, is 1.If I am not mistaken, this translates to
dt=1
for the EmuTN job runned by cloud services, which is not a viable value.I open this MR not for a blind merge, but rather to make sure the above interpretation of
sampling_rate
for EmuTnBackend is right, and reflect on a good default value.