-
Notifications
You must be signed in to change notification settings - Fork 62
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
[ML] Improvements to trend modelling and periodicity testing for forecasting #7
[ML] Improvements to trend modelling and periodicity testing for forecasting #7
Conversation
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 noticed one thing that probably slipped between the cracks of the previous reviews of incremental changes.
lib/model/CAnomalyDetector.cc
Outdated
@@ -91,7 +91,7 @@ CAnomalyDetector::TModelPtr makeModel(const CAnomalyDetector::TModelFactoryCPtr | |||
|
|||
// Increment this every time a change to the state is made that requires | |||
// existing state to be discarded | |||
const std::string CAnomalyDetector::STATE_VERSION("34"); | |||
const std::string CAnomalyDetector::STATE_VERSION("35"); |
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 don't think we should be doing this now the changed portion of the state is being upgraded.
Hopefully reverting this line won't mean regenerating any of the upgrade test files, but please can you check?
Thanks @droberts195 it had indeed: corrected. |
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
This was inadvertently added in #7
This was inadvertently added in #7
This was inadvertently added in #7
This was inadvertently added in #7
Description
This is a merge of a feature branch for issue #5. This work has already been reviewed as individual changes; however, I had some merge collisions with other recent changes.
Effects
This affects results, both model bounds and anomaly detection, for all data sets for which we use trend and/or seasonality modelling. In practice this is most data sets for non-rare functions. We've investigated changes across our full benchmark set. In all but one case, where results have changed appreciably, our anomaly detection detection accuracy has improved. Of course the primary driver is forecasting, which is now more reliable, has greatly improved confidence intervals and doesn't display obvious pathologies, particularly for longer time ranges.
This introduces a mean 30% memory increase across our benchmark set. The reasons for this are unavoidable and can be broken down as:
These costs can be offset against some recent memory wins going into 6.3.