[ML] DF Regression: fix custom results_field and prediction_field_name not considered in eval config #48599

alvarezmelissa87 · 2019-10-18T01:55:23Z

Summary

This PR takes into account a custom results_field and prediction_field_name that could be set by the user using the advanced editor when creating a regression job.

The custom fields are used if set when creating the payload for the /_evaluate endpoint. Default 'ml' results_field is used when one is not set. <dependent_variable>_prediction is used when prediction_field_name is not set.

Disables create button if job is regression type and dependent_variable has not been set

Checklist

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

This was checked for cross-browser compatibility, including a check against IE11
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

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~~

elasticmachine · 2019-10-18T01:55:25Z

Pinging @elastic/ml-ui (:ml)

elasticmachine · 2019-10-18T04:12:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4a788f2

walterra

Added a question about getPredictionFieldName().

walterra · 2019-10-18T07:09:35Z

x-pack/legacy/plugins/ml/public/data_frame_analytics/common/analytics.ts

@@ -81,6 +82,15 @@ export const getDependentVar = (analysis: AnalysisConfig) => {
  return depVar;
 };

+export const getPredictionFieldName = (analysis: AnalysisConfig) => {
+  // Default to dependent_variable


The comment here says Default to dependent_variable but I don't see that happening, is this comment meant as a TODO or does it happen somewhere else?

I'd also suggest having all comments as tsdoc for functions. More convenient to look up in IDE and let us generate the code documentation

Good one @walterra - I've clarified this comment in 695aaed

I just wanted to make it clear that if prediction_field_name is not defined/set it will be defaulted to dependent_variable when the config is created.

@darnautov - good suggestion, though in this case it's just more of an FYI comment for where this is used. Also I think TypeScript kind of accomplishes the same functionality by defining the parameters, return value, etc. 🤔

walterra · 2019-10-18T07:14:04Z

x-pack/legacy/plugins/ml/public/data_frame_analytics/common/analytics.ts

+  const query = { term: {} };
+  // @ts-ignore TODO: fix types here
+  query.term[`${resultsField}.is_training`] = { value: isTraining };


You could try to do the original assignment with the computed property name:

const query = { term: { [`${resultsField}.is_training`]: { value: isTraining } } }'

Nice suggestion! Updated in 695aaed

darnautov · 2019-10-18T08:13:23Z

x-pack/legacy/plugins/ml/public/data_frame_analytics/common/analytics.ts

@@ -81,6 +82,15 @@ export const getDependentVar = (analysis: AnalysisConfig) => {
  return depVar;
 };

+export const getPredictionFieldName = (analysis: AnalysisConfig) => {
+  // Default to dependent_variable


I'd also suggest having all comments as tsdoc for functions. More convenient to look up in IDE and let us generate the code documentation

darnautov · 2019-10-18T08:18:30Z

...c/data_frame_analytics/pages/analytics_management/components/analytics_list/expanded_row.tsx

+  getValuesFromResponse,
+  loadEvalData,
+  Eval,
+} from '../../../../common';
 import { isCompletedAnalyticsJob } from './common';
 import { isRegressionAnalysis } from '../../../../common/analytics';
 // import { ExpandedRowMessagesPane } from './expanded_row_messages_pane';


commented import (not part of the changes, but should be removed I guess)

I believe this was added in the original code for when the messages become available - which I think they are now if not very soon. I think it's worth keeping in as it will be used very soon. 😄

walterra

Latest changes LGTM

darnautov

LGTM

elasticmachine · 2019-10-18T14:53:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 695aaed

peteharverson

One comment about prediction_field_name needing to be set, but otherwise tested this and LGTM

peteharverson · 2019-10-18T15:06:39Z

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`;


When testing this I noticed that it is possible to create a job without entering the prediction_field_name. The Create button should be disabled unless they have entered a prediction_field_name.

The prediction_field_name is optional (defaults to whatever <dependent_variable>_prediction) - do you mean the dependent_variable? If so that was fixed in this commit : df6b171

elasticmachine · 2019-10-18T17:16:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: df6b171

elasticmachine · 2019-10-18T20:15:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 46608b1

elasticmachine · 2019-10-18T21:31:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: bdadabd

…e not considered in eval config (elastic#48599) * consider results_field and prediction_field_name in eval congif * temp typescript fix for evaluatePanel not being used yet * fix typescript error. clarify comment * Disable create button if dependentVariable not set * ensure advancedEditor json valid at every change * update reducer test

…e not considered in eval config (#48599) (#48714) * consider results_field and prediction_field_name in eval congif * temp typescript fix for evaluatePanel not being used yet * fix typescript error. clarify comment * Disable create button if dependentVariable not set * ensure advancedEditor json valid at every change * update reducer test

…e not considered in eval config (#48599) (#48713) * consider results_field and prediction_field_name in eval congif * temp typescript fix for evaluatePanel not being used yet * fix typescript error. clarify comment * Disable create button if dependentVariable not set * ensure advancedEditor json valid at every change * update reducer test

consider results_field and prediction_field_name in eval congif

147c392

alvarezmelissa87 added :ml v8.0.0 v7.5.0 Feature:Data Frame Analytics ML data frame analytics features labels Oct 18, 2019

alvarezmelissa87 requested review from walterra, darnautov, peteharverson and jgowdyelastic October 18, 2019 01:55

alvarezmelissa87 requested a review from a team as a code owner October 18, 2019 01:55

alvarezmelissa87 self-assigned this Oct 18, 2019

alvarezmelissa87 added the release_note:fix label Oct 18, 2019

temp typescript fix for evaluatePanel not being used yet

4a788f2

alvarezmelissa87 removed the request for review from jgowdyelastic October 18, 2019 02:57

walterra reviewed Oct 18, 2019

View reviewed changes

darnautov reviewed Oct 18, 2019

View reviewed changes

fix typescript error. clarify comment

695aaed

walterra approved these changes Oct 18, 2019

View reviewed changes

alvarezmelissa87 mentioned this pull request Oct 18, 2019

[ML] Meta - Regression UI #47890

Closed

46 tasks

darnautov approved these changes Oct 18, 2019

View reviewed changes

peteharverson approved these changes Oct 18, 2019

View reviewed changes

Disable create button if dependentVariable not set

df6b171

alvarezmelissa87 added 2 commits October 18, 2019 14:55

ensure advancedEditor json valid at every change

46608b1

update reducer test

bdadabd

alvarezmelissa87 merged commit 00c21dc into elastic:master Oct 19, 2019

This was referenced Oct 19, 2019

[7.x] [ML] DF Regression: fix custom results_field and prediction_field_name not considered in eval config (#48599) #48713

Merged

[7.5] [ML] DF Regression: fix custom results_field and prediction_field_name not considered in eval config (#48599) #48714

Merged

alvarezmelissa87 deleted the ml-regression-results-field branch October 21, 2019 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] DF Regression: fix custom results_field and prediction_field_name not considered in eval config #48599

[ML] DF Regression: fix custom results_field and prediction_field_name not considered in eval config #48599

alvarezmelissa87 commented Oct 18, 2019 •

edited

Loading

elasticmachine commented Oct 18, 2019

elasticmachine commented Oct 18, 2019

walterra left a comment

walterra Oct 18, 2019

darnautov Oct 18, 2019

alvarezmelissa87 Oct 18, 2019

walterra Oct 18, 2019

alvarezmelissa87 Oct 18, 2019

darnautov Oct 18, 2019

darnautov Oct 18, 2019

alvarezmelissa87 Oct 18, 2019

walterra left a comment

darnautov left a comment

elasticmachine commented Oct 18, 2019

peteharverson left a comment

peteharverson Oct 18, 2019

alvarezmelissa87 Oct 18, 2019

elasticmachine commented Oct 18, 2019

elasticmachine commented Oct 18, 2019

elasticmachine commented Oct 18, 2019

[ML] DF Regression: fix custom results_field and prediction_field_name not considered in eval config #48599

[ML] DF Regression: fix custom results_field and prediction_field_name not considered in eval config #48599

Conversation

alvarezmelissa87 commented Oct 18, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Oct 18, 2019

elasticmachine commented Oct 18, 2019

💚 Build Succeeded

walterra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

darnautov left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 18, 2019

💚 Build Succeeded

peteharverson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 18, 2019

💚 Build Succeeded

elasticmachine commented Oct 18, 2019

💔 Build Failed

elasticmachine commented Oct 18, 2019

💚 Build Succeeded

alvarezmelissa87 commented Oct 18, 2019 •

edited

Loading