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 Regression exploration: replace table with data grid #63650

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Apr 15, 2020

Summary

Related meta issue: #51288

  • replaces EuiInMemoryTable with EuiDataGrid component
  • replaces built in table search with QueryStringInput component

image

  • updates the way errors are displayed in the Evaluate panel

image

NOTE

Paging issue @peteharverson mentioned will be fixed in a follow up once @walterra 's fix has been merged in order to use the same solution.

Checklist

Delete any items that are not applicable to this PR.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@peteharverson
Copy link
Contributor

peteharverson commented Apr 16, 2020

Any idea why ml.is_training is showing as a text type field rather than boolean in the sort control? The mapping in the index pattern shows boolean.

image

@peteharverson - added 'boolean' schema for 'boolean' type fields in e825faa

@peteharverson
Copy link
Contributor

peteharverson commented Apr 16, 2020

Not needed here (as I think it used to show an error before too), but is there a way we can not show an error in this case, when filtering for training/testing data, and the doc count is zero as expected:

image

@peteharverson - good question. This particular error can be returned for reasons other than no documents being returned from the given query to run the evaluate analysis on so I defaulted to always showing it. One thing we could do is only show the error if the docCount is greater than 0. Happy to add it in a follow up as I'll need to do it for both regression/classification views.

@peteharverson
Copy link
Contributor

When the query bar query changes, we need to check the selected page is still valid. Here I was on the last page (125), edit the filter, and it looks like the grid is trying to reload page 125 even though there are only 24 pages for the new query value:

image

@peteharverson
Copy link
Contributor

peteharverson commented Apr 16, 2020

Can we prevent the user trying to sort on the object type ml.feature_importance field? If I try and sort on that, I get an error:

image

@peteharverson - good catch! Fixed in e825faa

Comment on lines +277 to +281
featureImportanceFields.push({
id: `${resultsField}.feature_importance`,
name: `${resultsField}.feature_importance`,
type: KBN_FIELD_TYPES.NUMBER,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this change, can you explain why having the individual fields is no longer necessary?

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Apr 16, 2020

Choose a reason for hiding this comment

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

The feature_importance field is an array of objects so the <results_field>.feature_importance.<id> field name wasn't actually mapping to anything - that column was just empty.
Setting it as just feature_importance lets the whole array show up in the popup.

@walterra
Copy link
Contributor

Regarding this comment by @peteharverson:

When the query bar query changes, we need to check the selected page is still valid.

In the parallel PR I'm working on to consolidate data grid code, I fixed it the following way: whenever the query changes, the paging will reset to the first page. https://github.com/elastic/kibana/pull/63447/files#diff-3a8487254a622f633414d662c4e7a84dR45

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.

Code LGTM.

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 edits and LGTM.

I think it would be nice to hide that 'error' shown in #63650 (comment) when filtering for testing / training, as long as we don't hide genuine errors which are causing the doc count to be zero.

Happy for the paging issue when the query changes the total number of pages to be done in a follow-up.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@alvarezmelissa87 alvarezmelissa87 merged commit dde3d96 into elastic:master Apr 16, 2020
@alvarezmelissa87 alvarezmelissa87 deleted the ml-dfanalytics-regression-results-datagrid branch April 16, 2020 17:50
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Apr 16, 2020
elastic#63650)

* add feature_importance column correctly

* wip: switch regression table to datagrid

* add search bar to regression view

* ensure feature importance fields show up correctly

* wip: filter by training/testing

* remove separate testing/training filter

* make error more clear

* handle lucene queries

* remove unnecessary comment

* ensure boolean shows up correctly.no sorting by feature importance

* remove unused translations
alvarezmelissa87 added a commit that referenced this pull request Apr 16, 2020
#63650) (#63739)

* add feature_importance column correctly

* wip: switch regression table to datagrid

* add search bar to regression view

* ensure feature importance fields show up correctly

* wip: filter by training/testing

* remove separate testing/training filter

* make error more clear

* handle lucene queries

* remove unnecessary comment

* ensure boolean shows up correctly.no sorting by feature importance

* remove unused translations
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.

5 participants