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

moving_avg forecasts should not include current point #11641

Merged
merged 1 commit into from Jun 22, 2015

Conversation

polyfractal
Copy link
Contributor

  • Forecasts should include all points up to, but not including, the current point.
  • Fixes some tests
  • Removes the "Gap" tests, which have proven to be super fragile due to accumulated edge cases, and aren't even very useful anymore because the mockHisto stuff generates randomly sized gaps.
  • Removes the concrete implementation of predict(), which makes things simpler / more intuitive

double[] predictions = new double[numPredictions];

// If there are no values, we can't do anything. Return an array of NaNs.
if (values.size() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

values.isEmpty()

@clintongormley clintongormley changed the title Aggregations: moving_avg forecasts should not include current point moving_avg forecasts should not include current point Jun 13, 2015
@@ -41,6 +42,29 @@
protected static final ParseField NAME_FIELD = new ParseField("linear");

@Override
public <T extends Number> double[] predict(Collection<T> values, int numPredictions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The two checks (values.size == 0 and numPredictions < 1) seem to be done in all the models, maybe we should move them to the super class and have the sub-classes implement doPredict() which is called from the predict() method in the super class?

Copy link
Contributor

Choose a reason for hiding this comment

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

in fact, to make all the models work in the same way, we could make MovAvgModel declare the following abstract method:

 public abstract <T extends Number> double[] next(Collection<T> values, int numForecasts);

Then the predict and next methods can just be declared in MovAvgModel and be the following:

    public final <T extends Number> double[] predict(Collection<T> values, int numPredictions) {
        return next(values, numPredictions);
    }

    public final <T extends Number> double next(Collection<T> values) {
        return next(values, 1)[0];
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with the first suggestion, rolling the checks/assertion into predict() which then calls down to doPredict().

I couldn't do the second suggestion, because not all the models follow that pattern. Simple / Linear / Ewma only know how to forecast one value at a time. E.g. they always return a single value, not an array. Which also means their prediction code needs to fill the array of predictions with that last value, whereas HL/HW generate new predictions at each point.

If we want less code c/p, going back to the old solution is probably best (concrete predict() for simple/linear/ewma and have HL/HW override it).

@polyfractal
Copy link
Contributor Author

@uboness @colings86 Sorry for delay, fixes applied

As an aside, the work I've been doing on the optimizer is going to need a fair amount of change to the next() and predict() interfaces...the current setup just isn't going to be ergonomic enough to deal with an optimizer changing model parameters constantly.

@colings86
Copy link
Contributor

LGTM

…apoint.

- Fixes tests, and removes a few special snowflake, fragile tests.
- Removes concrete implementation of predict() and moves it into
  each model so that the logic is clearer.  Because there is some
  shared checks/assertions, those remain in predict() and the main
  prediction happens in doPredict()
polyfractal added a commit that referenced this pull request Jun 22, 2015
Aggregations: Moving average forecasts should not include current point
@polyfractal polyfractal merged commit 2ebb44d into elastic:master Jun 22, 2015
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.

None yet

4 participants