-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[AnomalyDetection] Add univariate trackers #33994
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
…rable of AnomalyPrediction.
f9e9e32 to
56efa5b
Compare
56efa5b to
b2ffec8
Compare
|
r: @damccorm |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
Also includes minor fixes on Specifiable and univariate perf tests.
damccorm
left a comment
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.
Thanks! Feedback is all minor, otherwise LGTM
|
|
||
| class WindowMode(Enum): | ||
| """Enum representing the window mode for windowed trackers.""" | ||
| #: operating on all data points from the beginning. |
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 assume this doesn't mean we're buffering all data points we've ever seen, right?
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 think reading other things, the answer is yes, but it would be good to be explicit here.
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.
It really depends on algorithms and what statistics we talk about here.
For example, we don't need to store all data points to compute the mean in a landmark window: an naive way is to only store the number of data points and their sum.
However, for quantile, we will have to store all the data to compute the exact answer. There are some approximate algorithms for quantile that does not need to store all data points, but they are outside the scope of this current implementation.
That's why in WindowTracer, we don't explicitly declare a list to store all data points, because it may or may not need all the data points.
damccorm
left a comment
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
* Change prediction in AnomalyResult to predictions which is now an iterable of AnomalyPrediction. * Add mean, stdev and quantile trackers with tests. * Add docstrings * Fix lints * Make trackers specifiable. Also includes minor fixes on Specifiable and univariate perf tests. * Adjust class structures in trackers. Minor fix per feedback.
This is a follow-up PR of #33845.
We added tracker classes that will be used in the detectors and other components of the transform.