Skip to content
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

Merged
merged 4 commits into from
Feb 27, 2018
Merged

[ML] Improvements to trend modelling and periodicity testing for forecasting #7

merged 4 commits into from
Feb 27, 2018

Conversation

tveasey
Copy link
Contributor

@tveasey tveasey commented Feb 23, 2018

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:

  1. The trend model itself is significantly larger (rather than 1 we now have 8 at different time scales).
  2. We have the trend model all the time, previously this was created if we determined it was worthwhile to do so, but we need this model for forecasting and can't create it retrospectively.
  3. The new style periodicity testing uses slightly more memory.
  4. We have some extra state in the seasonal components for forecasting.
  5. We sometimes find additional periodic components in the data (with associated accuracy benefits).

These costs can be offset against some recent memory wins going into 6.3.

Copy link
Contributor

@droberts195 droberts195 left a 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.

@@ -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");
Copy link
Contributor

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?

@tveasey
Copy link
Contributor Author

tveasey commented Feb 27, 2018

Thanks @droberts195 it had indeed: corrected.

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tveasey tveasey merged commit 64fb093 into elastic:master Feb 27, 2018
tveasey added a commit that referenced this pull request Mar 8, 2018
…ng (#7)

This is a merge of a feature branch for issue #5.
droberts195 added a commit that referenced this pull request Mar 9, 2018
This was inadvertently added in #7
droberts195 added a commit that referenced this pull request Mar 9, 2018
This was inadvertently added in #7
@sophiec20 sophiec20 changed the title Improvements to trend modelling and periodicity testing for forecasting [ML] Improvements to trend modelling and periodicity testing for forecasting Mar 28, 2018
@sophiec20 sophiec20 added the :ml label Apr 4, 2018
droberts195 pushed a commit that referenced this pull request Apr 23, 2018
…ng (#7)

This is a merge of a feature branch for issue #5.
droberts195 added a commit that referenced this pull request Apr 23, 2018
This was inadvertently added in #7
droberts195 pushed a commit that referenced this pull request Apr 23, 2018
…ng (#7)

This is a merge of a feature branch for issue #5.
droberts195 added a commit that referenced this pull request Apr 23, 2018
This was inadvertently added in #7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants