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][CI] fix integration test after change in ml-cpp#327 #36296

Closed
wants to merge 1 commit into from

Conversation

hendrikmuhs
Copy link

fix integration test after change in ml-cpp#327

fixes: #36258

@hendrikmuhs hendrikmuhs added >test Issues or PRs that are addressing/adding tests v7.0.0 :ml Machine learning v6.6.0 v6.5.3 labels Dec 6, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@droberts195
Copy link
Contributor

The more I look at this the more I wonder whether elastic/ml-cpp#327 was really the right thing to do.

The date_histogram aggregation returns bucket timestamps that are the start of each bucket, which is what the ML C++ used to do for all result types too. So why was there an offset in what was displayed on the chart? Maybe it's because the aggregation used to get the raw data was using an offset in the date_histogram config, but the query to plot the model plot line wasn't? If that's the explanation then an alternative fix would have been to also use an offset when getting the model plot values to plot.

If we stick with the approach of changing the results then I think a docs change is required too. https://www.elastic.co/guide/en/elasticsearch/reference/current/ml-results-resource.html says:

The timestamp for the results is the start of the bucket time interval.

That sentence needs changing to:

The timestamp for the results is the start of the bucket time interval, except for model plot and forecast results generated by version 6.5.3 and above, where the timestamp is the mid-point of the bucket time interval.

I also wonder if this difference in result timestamps for different types of results for the same bucket is going to bite us in some other obscure way, like if there's some other place in the code where we're assuming that we can get all the results relating to a particular bucket with a query that asks for the exact bucket time.

Sorry to bring this up after elastic/ml-cpp#327 is already merged. I should have looked at that more closely when it was in progress.

cc @tveasey @peteharverson @sophiec20

@droberts195
Copy link
Contributor

Following elastic/ml-cpp#332 this PR is no longer required. Once elastic/ml-cpp#332 is backported to 6.5.4 and has propagated through the build system I'll open a new PR to revert the @AwaitsFix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml Machine learning >test Issues or PRs that are addressing/adding tests v6.5.3 v6.6.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] ForecastIT#testSingleSeries fails reproducibly on master
4 participants