[ML] DF Analytics Classification: ensure confusion matrix can be fetched #53629

alvarezmelissa87 · 2019-12-19T20:19:50Z

Summary

NOTE This will need to be updated with additional changes (elastic/elasticsearch#50219) to the endpoint that just went in. I'll ping when it's ready for another review. Apologies
-- This has been updated and is ready for a final look.

Add dependent_variable field type check before hitting the _evaluate endpoint for the confusion matrix.

Accounts for changes to the _evaluate endpoint introduced in elastic/ml-cpp#877 in which the .keyword suffix will no longer be needed for boolean or integer dependent variable fields

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-12-19T20:19:53Z

Pinging @elastic/ml-ui (:ml)

jgowdyelastic · 2019-12-20T08:25:29Z

...alytics/pages/analytics_exploration/components/classification_exploration/evaluate_panel.tsx

+        await newJobCapsService.initializeFromIndexPattern(indexPattern);
+        // Check dependent_variable field type to see if .keyword suffix is required for evaluate endpoint
+        const { fields } = newJobCapsService;
+        const depVarFieldType = fields.find((field: any) => field.name === dependentVariable)?.type;


By saying this field is any you're losing the type information that you get from free from the fields array.
there is no need to add a type here at all as it knows it's a Field[]

Good point - removed in 0a79e2a39475beff24b51a40e78f6d6d3e806cca

jgowdyelastic · 2019-12-20T08:29:23Z

...alytics/pages/analytics_exploration/components/classification_exploration/evaluate_panel.tsx

@@ -37,8 +38,12 @@ import {
 } from '../../../../common/analytics';
 import { LoadingPanel } from '../loading_panel';
 import { getColumnData } from './column_data';
+import { useKibanaContext } from '../../../../../contexts/kibana';
+import { newJobCapsService } from '../../../../../services/new_job_capabilities_service';
+import { getIndexPatternIdFromName } from '../../../../../../application/util/index_utils';


this goes one level too deep. it doesn't need to go outside application

Thanks! Updated in 0a79e2a39475beff24b51a40e78f6d6d3e806cca

jgowdyelastic · 2019-12-20T08:31:18Z

...alytics/pages/analytics_exploration/components/classification_exploration/evaluate_panel.tsx


 const defaultPanelWidth = 500;
+const NO_KEYWORD_FIELDTYPES = ['boolean', 'integer'];


should be
const NO_KEYWORD_FIELDTYPES = [ES_FIELD_TYPES.BOOLEAN, ES_FIELD_TYPES.INTEGER];

Changed in 0a79e2a39475beff24b51a40e78f6d6d3e806cca

peteharverson

One question on the IndexPattern import, but otherwise LGTM

peteharverson · 2019-12-20T12:26:38Z

...alytics/pages/analytics_exploration/components/classification_exploration/evaluate_panel.tsx

@@ -7,6 +7,7 @@
 import React, { FC, useState, useEffect, Fragment } from 'react';
 import { i18n } from '@kbn/i18n';
 import { FormattedMessage } from '@kbn/i18n/react';
+import { IndexPattern } from 'ui/index_patterns';


Looking at kibana_context.ts can this import be from src/plugins/data/public - IndexPatternsContract ?

I believe the type should be IIndexPattern from the same location.
I plan to change all of our uses of IndexPattern to be of that type in future NP work.

Updated 0a79e2a39475beff24b51a40e78f6d6d3e806cca

peteharverson

Latest edits LGTM

…point

jgowdyelastic

LGTM

peteharverson

Running against the latest ES mini stack build, the confusion matrix isn't being displayed for the classification jobs I've run.

From the call to the _evaluate endpoint, I see errors in the network tab of the form:

{"statusCode":400,"error":"Bad Request","message":"[status_exception] No documents found containing both [stabf, ml.stabf_prediction.keyword] fields"}

and a call to the es_search endpoint is failing with:

{"statusCode":400,"error":"Bad Request","message":"[query_shard_exception] No mapping found for [ml.stabf_prediction.keyword] in order to sort on, with { index_uuid=\"1-YoKSD5TWGllOcCrGZCoQ\" & index=\"electrical_grid_stab_class\" }"}

peteharverson

Tested latest edits and LGTM

kibanamachine · 2020-01-08T17:50:26Z

💚 Build Succeeded

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

History

💚 Build #18430 succeeded c22a2ab
💔 Build #18410 failed 6c6689cd47edf6b45670b69390ce6da4376d3b78
💚 Build #17365 succeeded 4f4a4eb3b76bc46c4fd62b4331297dbec4878287
💚 Build #17044 succeeded 0a79e2a39475beff24b51a40e78f6d6d3e806cca

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…hed (elastic#53629) * check depVar field type before adding keyword suffix for evaluate endpoint * update indexPattern type and use FIELD types * add keyword suffix if field type is keyword * keyword suffix added if depVar is of type keyword AND text

…hed (#53629) (#54288) * check depVar field type before adding keyword suffix for evaluate endpoint * update indexPattern type and use FIELD types * add keyword suffix if field type is keyword * keyword suffix added if depVar is of type keyword AND text

* master: (23 commits) [Vis: Default editor] Reactify the timelion editor (elastic#52990) [Discover] fix histogram min interval (elastic#53979) [Telemetry] [Monitoring] Only retry fetching usage once monito… (elastic#54309) [docs][APM] Add runtime index config documentation (elastic#53907) [SIEM] Detection engine timeline (elastic#53783) Filter scripted fields preview field list to source fields (elastic#53826) Management - New platform api (elastic#52579) Reset region and Account when switching inventory (elastic#54287) [SIEM] [Case] Case workflow api schema (elastic#51535) Code coverage setup on CI (elastic#49003) [ML] DF Analytics Results: adds link to docs (elastic#54189) Update schemas boolean, byteSize, and duration to coerce strings (elastic#54177) [Metrics UI] Pass relevant shouldAllowEdit capabilities into SettingsPage (elastic#49781) [Canvas] Fixes bugs with autoplay and refresh (elastic#53149) [ML] DF Analytics Classification: ensure confusion matrix can be fetched (elastic#53629) Fix Vega react eslint errors (elastic#54259) Remove non existing codeowners (elastic#54274) use correct type (elastic#54244) [Dashboard] Removing 100% as dshDashboardViewport height (elastic#54263) add `examples/` to no-restricted-path config (elastic#54252) ...

alvarezmelissa87 added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Data Frame Analytics ML data frame analytics features v7.6.0 labels Dec 19, 2019

alvarezmelissa87 requested a review from a team as a code owner December 19, 2019 20:19

alvarezmelissa87 self-assigned this Dec 19, 2019

alvarezmelissa87 mentioned this pull request Dec 19, 2019

[ML] Meta - Classification UI #51310

Closed

22 tasks

alvarezmelissa87 requested review from peteharverson, jgowdyelastic, darnautov and walterra December 19, 2019 20:25

jgowdyelastic reviewed Dec 20, 2019

View reviewed changes

peteharverson approved these changes Dec 20, 2019

View reviewed changes

alvarezmelissa87 force-pushed the ml-df-classification-evaluate-update branch from 0a79e2a to 4f4a4eb Compare December 28, 2019 20:56

elastic deleted a comment from elasticmachine Dec 28, 2019

peteharverson approved these changes Jan 2, 2020

View reviewed changes

alvarezmelissa87 force-pushed the ml-df-classification-evaluate-update branch from 4f4a4eb to 6c6689c Compare January 7, 2020 15:05

alvarezmelissa87 added 3 commits January 7, 2020 10:55

check depVar field type before adding keyword suffix for evaluate end…

c95fcb2

…point

update indexPattern type and use FIELD types

4b52718

add keyword suffix if field type is keyword

c22a2ab

alvarezmelissa87 force-pushed the ml-df-classification-evaluate-update branch from 1b12463 to c22a2ab Compare January 7, 2020 15:56

jgowdyelastic approved these changes Jan 7, 2020

View reviewed changes

peteharverson self-requested a review January 8, 2020 11:48

peteharverson reviewed Jan 8, 2020

View reviewed changes

keyword suffix added if depVar is of type keyword AND text

d1cc510

peteharverson approved these changes Jan 8, 2020

View reviewed changes

alvarezmelissa87 merged commit fc948a0 into elastic:master Jan 8, 2020

alvarezmelissa87 mentioned this pull request Jan 8, 2020

[7.x] [ML] DF Analytics Classification: ensure confusion matrix can be fetched (#53629) #54288

Merged

alvarezmelissa87 deleted the ml-df-classification-evaluate-update branch January 8, 2020 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] DF Analytics Classification: ensure confusion matrix can be fetched #53629

[ML] DF Analytics Classification: ensure confusion matrix can be fetched #53629

alvarezmelissa87 commented Dec 19, 2019 •

edited

Loading

elasticmachine commented Dec 19, 2019

jgowdyelastic Dec 20, 2019

alvarezmelissa87 Dec 20, 2019

jgowdyelastic Dec 20, 2019

alvarezmelissa87 Dec 20, 2019

jgowdyelastic Dec 20, 2019 •

edited

Loading

alvarezmelissa87 Dec 20, 2019

peteharverson left a comment

peteharverson Dec 20, 2019

jgowdyelastic Dec 20, 2019

alvarezmelissa87 Dec 20, 2019

peteharverson left a comment

jgowdyelastic left a comment

peteharverson left a comment

peteharverson left a comment

kibanamachine commented Jan 8, 2020


		const defaultPanelWidth = 500;
		const NO_KEYWORD_FIELDTYPES = ['boolean', 'integer'];

[ML] DF Analytics Classification: ensure confusion matrix can be fetched #53629

[ML] DF Analytics Classification: ensure confusion matrix can be fetched #53629

Conversation

alvarezmelissa87 commented Dec 19, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Dec 19, 2019

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

jgowdyelastic Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 8, 2020

💚 Build Succeeded

History

alvarezmelissa87 commented Dec 19, 2019 •

edited

Loading

jgowdyelastic Dec 20, 2019 •

edited

Loading