-
Notifications
You must be signed in to change notification settings - Fork 117
Fix math domain error in decaying poll rate #152
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
* Check that `self._a` is not smaller that a minimum allowed value.
|
|
||
|
|
||
| class PollRateFunction: | ||
| _min_a = 0.01 |
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.
Does it make sense to have this variable defined in the settings.py?
Or Have it defined capitalized at the beginning of the file?
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.
@victorusu Generally, yes, but not in this issue. I will create another one.
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.
I added this one so that I do not enter a magic number in the middle of the code.
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.
@victorusu Another problem with exposing _min_a as-is is that it doesn't have a physical meaning. It is not the minimum poll rate.
| self._b = self._min_rate | ||
| self._a = init_rate - self._b | ||
| if self._a < self._min_a: | ||
| self._a = self._min_a |
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.
I disagree with this solution. _min_a has no meaning and it is arbitrary. The problem here is that the initial rate is less than the minimum rate. Ideally, we would like to change the function and make it an increasing logarithmic. Since this is a bit too much, I would suggest the following:
if abs(self._min_rate - init_rate) < eps:
self._a = 0
self._c = 0
else:
# as of nowThe above function will make the subsequent calls to __call__() return always the minimum rate. The eps should have such a value to guarantee that math.log() doesn't throw.
|
@jenkins-cscs retry daint |
self._ais not smaller that a minimum allowed value.Fixes #77