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] DF Analytics: create classification jobs results view #52584

Merged

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Dec 9, 2019

Summary

NOTE: This will need to be updated to account 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. Once the ES build contains the change I'll be able to test it.
Added this to the meta issue to do in a follow-up: #51310
cc @peteharverson, @walterra

Related meta issue: #51310

  • enables View link in Analytics jobs list for classification jobs
  • results view displays confusion matrix result from _evaluate endpoint
  • results view displays a table containing data from the classification job destination index.
    • filter by training/testing data
    • can select additional columns
    • can change sort order of columns
    • can search the contents of the table - a search fetches another 1000 records

image

  • cell popover content reveals how the normalized value was calculated

image

  • _evaluate endpoint is rerun with search queries executed in the results table

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

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.

Great update! I added some suggestions/questions mainly about types.

@walterra - thanks for taking a look! All type updates added here: f5bb0a7

});
});

const columns: any = [
Copy link
Contributor

Choose a reason for hiding this comment

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

const [confusionMatrixData, setConfusionMatrixData] = useState<
ClassificationEvaluateResponse['classification']['multiclass_confusion_matrix']['confusion_matrix']
>([]);
const [columns, setColumns] = useState<any>([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

},
});
}
}, [confusionMatrixData]);
Copy link
Contributor

Choose a reason for hiding this comment

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

To do a deep equal, maybe JSON.stringify(confusionMatrixData) is worth doing here similar to JSON.stringify(searchQuery) further down.

@sophiec20
Copy link
Contributor

sophiec20 commented Dec 10, 2019

Feedback on look and feel of the confusion matrix (but I don't think this should prevent the merge as it can be a follow up).

Using the full width for the confusion matrix table makes it hard to read for wide screens. It is useful to be able to compare adjacent row and column values. When stretched across the page, this becomes more difficult.

@walterra
Copy link
Contributor

walterra commented Dec 10, 2019

When resizing one of the confusion matrix columns, the data grid expands beyond the page width, this also then affects the results table below:

image

Might be related to this data grid issue: elastic/eui#2618

When resizing the page, the confusion matrix data grid also doesn't update it's width.

@walterra - looks like it is related to the data grid issue you linked above. I've added a fix for the column resize affecting the results table. I've set the evalute panel width to the results table width - this prevents the data grid resizing from affecting the results table and ensures the evalute panel has a fixed width (f0a909d)

The data grid ignoring the parent width issue is still seen but I'll do a bit more digging on that.

@walterra
Copy link
Contributor

walterra commented Dec 10, 2019

When clicking on the popover button of a cell in the confusion matrix, I'm getting this in the console:

image

Not sure if we're using EuiDataGrid not in the right way or if it's a bug with EuiDataGrid. When wrapping the setCellProps call in setTimeout the error doesn't trigger.

Heya @walterra - looks like this was being caused due to the setCellProps being called directly instead of from a lifecycle method. Fix is to wrap it in a useEffect as the renderCellValue is treated as a functional component behind the scenes. Fixed here: bf48412

@walterra
Copy link
Contributor

The first time I run a search in the results table, I get this error in the console related to EuiDataGrid:

image

Looks like the same issue described above with calling setCellProps. When I used the setTimeout workaround I get this error though:

image

@walterra
Copy link
Contributor

walterra commented Dec 10, 2019

With some searches I end up with an empty page and errors like this:

image

It doesn't happen with every type of search. For example, note that the search worked with e.g. currency:EUR from the previous comment. The sceenshot was taken when I searched for customer_id:44 on a job based on the kibana ecommerce dataset.

@walterra - nice catch! This was occurring because there was an assumption that there would always be a correct class and incorrect class predicted. In some cases there is only the correctly predicted class in the confusion matrix. I've updated to check for that here 81e4b50

@alvarezmelissa87
Copy link
Contributor Author

retest

@walterra
Copy link
Contributor

Regarding setCellProps/useEffect: Linting complains that is doesn't recognize renderCellValue as a React component. If I change renderCellValue to the following linting doesn't complain anymore:

  const CellValue: FC<{
    rowIndex: number;
    columnId: string;
    setCellProps: any;
  }> = ({ rowIndex, columnId, setCellProps }) => {
    const cellValue = columnsData[rowIndex][columnId];
    useEffect(() => {
      setCellProps({
        style: {
          backgroundColor: `rgba(0, 179, 164, ${cellValue})`,
        },
      });
    }, [cellValue, setCellProps]);
    return (
      <span>{typeof cellValue === 'number' ? `${Math.round(cellValue * 100)}%` : cellValue}</span>
    );
  };

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, just added a suggestion above regarding the linting problem with useEffect().

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 latest changes and all LGTM.

I prefer the confusion matrix not going full width. Looks better as the table only has the two columns.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@alvarezmelissa87 alvarezmelissa87 merged commit 0cd5bb0 into elastic:master Dec 12, 2019
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Dec 12, 2019
…2584)

* wip: create classification results page + table and evaluate panel

* enable view link for classification jobs

* wip: fetch classification eval data

* wip: display confusion matrix in datagrid

* evaluate panel: add heatmap for cells and doc count

* Update use of loadEvalData in expanded row component

* Add metric type for evaluate endpoint and fix localization error

* handle no incorrect prediction classes case for confusion matrix. remove unused translation

* setCellProps needs to be called from a lifecycle method - wrap in useEffect

* TypeScript improvements

* fix datagrid column resize affecting results table

* allow custom prediction field for classification jobs

* ensure values are rounded correctly and add tooltip

* temp workaroun for datagrid width issues
@alvarezmelissa87 alvarezmelissa87 deleted the ml-classifications-results branch December 12, 2019 19:04
alvarezmelissa87 added a commit that referenced this pull request Dec 12, 2019
…52941)

* wip: create classification results page + table and evaluate panel

* enable view link for classification jobs

* wip: fetch classification eval data

* wip: display confusion matrix in datagrid

* evaluate panel: add heatmap for cells and doc count

* Update use of loadEvalData in expanded row component

* Add metric type for evaluate endpoint and fix localization error

* handle no incorrect prediction classes case for confusion matrix. remove unused translation

* setCellProps needs to be called from a lifecycle method - wrap in useEffect

* TypeScript improvements

* fix datagrid column resize affecting results table

* allow custom prediction field for classification jobs

* ensure values are rounded correctly and add tooltip

* temp workaroun for datagrid width issues
timductive pushed a commit to timductive/kibana that referenced this pull request Dec 16, 2019
…2584)

* wip: create classification results page + table and evaluate panel

* enable view link for classification jobs

* wip: fetch classification eval data

* wip: display confusion matrix in datagrid

* evaluate panel: add heatmap for cells and doc count

* Update use of loadEvalData in expanded row component

* Add metric type for evaluate endpoint and fix localization error

* handle no incorrect prediction classes case for confusion matrix. remove unused translation

* setCellProps needs to be called from a lifecycle method - wrap in useEffect

* TypeScript improvements

* fix datagrid column resize affecting results table

* allow custom prediction field for classification jobs

* ensure values are rounded correctly and add tooltip

* temp workaroun for datagrid width issues
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

5 participants