-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Change maximum smoothing factor from 1 to 0.999. #1974
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
Conversation
1 as a smoothing factor always leads to a constant value of 0 being plotted. 0.999 is the actual highest value you can select for meaningful smoothing.
|
@stephanwlee PTAL |
|
Thank you for submitting this PR @flostim! This seems reasonable as a smoothing factor of 1 is indeed not useful (are there any scenarios where it might be?). One question I have is whether users will be confused about the slider not going all the way to 1. Additionally, while we're on this topic, do we really need 3 decimal places here? Out of curiosity, are you specifically testing different smoothing factors or are you just trying to make it easier to set it to the maximum reasonable value quickly? |
|
Thanks for having a look!
I don't know of any scenarios where it would be useful.
Hmm, that's a fair point but I guess if they know that TB is using exponential smoothing they could see that the smoothing factor has to be <1.
I wouldn't want to remove functionality here and I definitely sometimes want to have stronger smoothing than 0.99 to get almost an overall average of the scalar.
It's mainly that I sometimes want to go to a really high smoothing factor, e.g. to see an overall trend and then it gets set to 1 and I have to go back to have effective smoothing. Very minor annoyance, but as I don't see a reasonable use-case for smoothing factor 1 I thought it could be avoided. |
Just to provide a bit of context: this was known at the time of |
Interesting. I can kind of see a reason to have smoothing=1.0 for the old algorithm but in the new case it just overlays the x-axis. |
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, thank you!
|
Thanks for the review! |
|
Hey @GalOshri, can you merge this, as I don't have write access. Thanks! |
|
Yep, was just waiting to see if anyone else had questions. Thank you for making this change! |
1 as a smoothing factor always leads to a constant value of 0 being plotted.
0.999 is the actual highest value you can select for meaningful smoothing.