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.

Sign up for GitHub

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

Jump to bottom

[ML] DF Analytics Regression results: rerun evaluate endpoint for search bar queries #50991

Merged
merged 7 commits into from
Nov 21, 2019
Merged

[ML] DF Analytics Regression results: rerun evaluate endpoint for search bar queries #50991

merged 7 commits into from
Nov 21, 2019

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Nov 18, 2019 •
edited
Loading

Summary

  • Reduce font for evaluate panel in regressions results view.
  • Add doc count for Training/Generalization errors.

image

Fetch regression job results from _evaluate endpoint for queries created by the results table search bar.

  • If search bar is not used to filter by is_training, re-fetch results for both training/generalization for queried data. Doc count is updated.

image

  • If search bar is used to filter by is_training, fetch results for queried data only for selected training subset (training or generalization). Doc count is also updated.

image

  • Provides helpful error message when invalid query syntax is used in the search bar

image

  • Adds tooltip explanations for MSE and rSquared

image

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner November 18, 2019 23:28
@alvarezmelissa87 alvarezmelissa87 self-assigned this Nov 18, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alvarezmelissa87 alvarezmelissa87 changed the title WIP: [ML] DF Analytics Regression results: rerun evaluate endpoint for search bar queries [ML] DF Analytics Regression results: rerun evaluate endpoint for search bar queries Nov 20, 2019
@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Nov 20, 2019
Closed
46 tasks
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

walterra
walterra reviewed Nov 20, 2019
View reviewed changes
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Looks good overall, added a bunch of TS related questions/suggestions.

x-pack/legacy/plugins/ml/public/data_frame_analytics/common/analytics.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/ml/public/data_frame_analytics/common/analytics.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/ml/public/data_frame_analytics/common/analytics.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/ml/public/data_frame_analytics/common/analytics.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/ml/public/data_frame_analytics/common/analytics.ts Outdated Show resolved Hide resolved
...e_analytics/pages/analytics_exploration/components/regression_exploration/evaluate_panel.tsx Outdated Show resolved Hide resolved
...e_analytics/pages/analytics_exploration/components/regression_exploration/evaluate_panel.tsx Outdated Show resolved Hide resolved
...me_analytics/pages/analytics_exploration/components/regression_exploration/evaluate_stat.tsx Outdated Show resolved Hide resolved
...e_analytics/pages/analytics_exploration/components/regression_exploration/evaluate_panel.tsx Outdated Show resolved Hide resolved
@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner November 20, 2019 19:57
cchaos
cchaos reviewed Nov 20, 2019
View reviewed changes
...s/pages/analytics_exploration/components/regression_exploration/_regression_exploration.scss Outdated
@@ -0,0 +1,3 @@
.mlDataFrameAnalyticsRegression__evaluateStat span.euiToolTipAnchor {
Copy link
Contributor

Choose a reason for hiding this comment

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

@alvarezmelissa87 Don't forget that you can pass custom classnames to EuiToolTip anchors with the prop anchorClassName. It's safer than targeting the .eui class directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder 😄 Updated to use anchorClassName instead - 629d3fd

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alvarezmelissa87
Copy link
Contributor Author

retest

@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Nov 20, 2019
Merged
3 tasks
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

peteharverson
peteharverson reviewed Nov 21, 2019
View reviewed changes
x-pack/legacy/plugins/ml/public/data_frame_analytics/common/analytics.ts
}) => {
const results: LoadEvaluateResult = { success: false, eval: null, error: null };
const defaultPredictionField = `${dependentVariable}_prediction`;
const predictedField = `${resultsField}.${
predictionFieldName ? predictionFieldName : defaultPredictionField
}`;

const query = { term: { [`${resultsField}.is_training`]: { value: isTraining } } };
const query = getEvalQueryBody({ resultsField, isTraining, searchQuery, ignoreDefaultQuery });
Copy link
Contributor

Choose a reason for hiding this comment

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

When the query matches just one document, I've seen that r_squared is returned by the evaluate endpoint as -Infinity. Can you check if this is correct, and if so, getValuesFromResponse needs to handle this case as currently it throws an error when calling toPrecision and the stats show up as -- (the mean_squared_error is a value and could be displayed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is caused because if there is only one doc variance is 0 - this was then being divided by 0 in the calculation to get rSquared. Instead of dividing by 0, 0 should just be returned.

A fix for this is up already elastic/elasticsearch#49439

We shouldn't need to do any additional checking in the UI.

peteharverson
peteharverson approved these changes Nov 21, 2019
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested with latest changes and LGTM.

When we switch the EUI table search bar to a KQL one, we should consider changing the layout to move the error stats into the main panel below the search bar. This will make it clearer that these errors relate to the same search as the table, and remove the duplication of the job ID / status which is duplicated in the current two panels.

walterra
walterra approved these changes Nov 21, 2019
View reviewed changes
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM 🎈

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alvarezmelissa87 alvarezmelissa87 merged commit 7ca2349 into elastic:master Nov 21, 2019
@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Nov 21, 2019
Merged
@alvarezmelissa87 alvarezmelissa87 deleted the ml-regression-updates branch November 21, 2019 17:54
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Nov 21, 2019
@alvarezmelissa87
[ML] DF Analytics Regression results: rerun evaluate endpoint for sea…
1dd4fcd 
…rch bar queries (elastic#50991)

* update evaluate panel values

* re-evaluate endpoint with subquery

* wip: add document count for training/gen evaluate

* add document count for training/gen evaluate continued

* use toolTipIcon, remove unnecessary comment

* typescript improvements

* use anchorClassName instead of targetin eui class directly
alvarezmelissa87 added a commit that referenced this pull request Nov 21, 2019
@alvarezmelissa87
[ML] DF Analytics Regression results: rerun evaluate endpoint for sea…
95f0068 
…rch bar queries (#50991) (#51316)

* update evaluate panel values

* re-evaluate endpoint with subquery

* wip: add document count for training/gen evaluate

* add document count for training/gen evaluate continued

* use toolTipIcon, remove unnecessary comment

* typescript improvements

* use anchorClassName instead of targetin eui class directly
@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Dec 2, 2019
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos left review comments

@peteharverson peteharverson peteharverson approved these changes

@walterra walterra walterra approved these changes

@jgowdyelastic jgowdyelastic Awaiting requested review from jgowdyelastic

@darnautov darnautov Awaiting requested review from darnautov

Assignees

@alvarezmelissa87 alvarezmelissa87

Labels
Feature:Data Frame Analytics ML data frame analytics features :ml release_note:enhancement v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alvarezmelissa87 @elasticmachine @walterra @cchaos @peteharverson