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] Data Frame Analytics: Adds scatterplot matrix to regression/classification results pages. #88353

Merged
merged 9 commits into from
Feb 1, 2021

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Jan 14, 2021

Review note for Kibana Design Team: The github review assignment got triggered by a SCSS file rename.

Summary

Part of #84420

  • Adds support for scatterplot matrices to regression/classification results pages
  • Lazy loads the scatterplot matrix including Vega code using Suspense. The approach is taken from the Kibana Vega plugin, creating this separate bundle means you'll load the 600kb+ Vega code only on pages where actually needed and not e.g. already on the analytics job list. Note for reviews: The file scatterplot_matrix_view.tsx did not change besides the default export, it just shows up as a new file because of the refactoring to support lazy loading.
  • Adds support for analytics configuration that use the excludes instead of includes field list.
  • Adds the field used for color legends to tooltips.

image

Checklist

For maintainers

@kertal
Copy link
Member

kertal commented Jan 15, 2021

randomly clicked here, so here is just a mini review: looks very nice!

@walterra walterra requested a review from pheyos January 25, 2021 11:39
@walterra walterra marked this pull request as ready for review January 25, 2021 11:40
@walterra walterra requested review from a team as code owners January 25, 2021 11:40
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Functional tests LGTM, great to have them in as part of this PR 🎉

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 and overall looks good. Only issue I found was that the page query isn't being used - which also applies for outlier jobs.

Not for this PR, but with the extra components being added to this views, we're almost at a point where we need to consider some sort of 'jump to' type links.

setIsLoading(false);
}
} catch (e) {
// TODO error handling
Copy link
Member

Choose a reason for hiding this comment

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

Is this todo for a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it just shows up as a new line because the file was moved and git didn't recognize it was a move. Better error handling is in the list of follow ups in the meta issue here #84420

</EuiFlexItem>
<EuiFlexItem style={{ width: '200px' }} grow={false}>
<EuiFormRow
label={i18n.translate('xpack.ml.splom.SampleSizeLabel', {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Sample is capitalized here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1ab0754.

</EuiFlexItem>
<EuiFlexItem style={{ width: '120px' }} grow={false}>
<EuiFormRow
label={i18n.translate('xpack.ml.splom.RandomScoringLabel', {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Random is capitalized here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1ab0754.

@qn895
Copy link
Member

qn895 commented Jan 27, 2021

Gave this a good test and was able to reproduce Pete's problem where the query or the Training/Test groups were not updating the plots.

Also, just wondering if we modify the colors to be more contrasting, specifically for binary classification analytics. For example, the current order makes it harder to distinguish yes or no in the charts since the two first colors are so similar.
Screen Shot 2021-01-26 at 17 59 32

Another option is to make the colors bolder, like what we have with the Elastic charts.
Screen Shot 2021-01-26 at 18 09 13 Screen Shot 2021-01-26 at 18 09 17

Another minor one which I'm pretty sure is not due to your PR, when the div overflows, for some reason on Chrome-based browsers the scroll bar is black:
Screen Shot 2021-01-26 at 18 03 35

@walterra
Copy link
Contributor Author

@peteharverson @qn895

  • Pushed a fix to apply the query from the search bar on all pages (wizard, outlier/regression/classification results).
  • Added a note to the meta issue to improve the color legend with more contrast for a follow up [ML] Data Frame Analytics: Scatterplot Matrix #84420

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

@qn895
Copy link
Member

qn895 commented Jan 27, 2021

Also tested and LGTM 🎉

@walterra
Copy link
Contributor Author

walterra commented Feb 1, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1730 1734 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 6.5MB 6.6MB +8.7KB
Unknown metric groups

async chunk count

id before after diff
ml 19 20 +1

History

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

@walterra walterra merged commit fb19aab into elastic:master Feb 1, 2021
@walterra walterra deleted the ml-scatterplot-results-pages branch February 1, 2021 11:31
walterra added a commit to walterra/kibana that referenced this pull request Feb 1, 2021
…sification results pages. (elastic#88353)

- Adds support for scatterplot matrices to regression/classification results pages
- Lazy loads the scatterplot matrix including Vega code using Suspense. The approach is taken from the Kibana Vega plugin, creating this separate bundle means you'll load the 600kb+ Vega code only on pages where actually needed and not e.g. already on the analytics job list. Note for reviews: The file scatterplot_matrix_view.tsx did not change besides the default export, it just shows up as a new file because of the refactoring to support lazy loading.
- Adds support for analytics configuration that use the excludes instead of includes field list.
- Adds the field used for color legends to tooltips.
walterra added a commit that referenced this pull request Feb 1, 2021
…sification results pages. (#88353) (#89848)

- Adds support for scatterplot matrices to regression/classification results pages
- Lazy loads the scatterplot matrix including Vega code using Suspense. The approach is taken from the Kibana Vega plugin, creating this separate bundle means you'll load the 600kb+ Vega code only on pages where actually needed and not e.g. already on the analytics job list. Note for reviews: The file scatterplot_matrix_view.tsx did not change besides the default export, it just shows up as a new file because of the refactoring to support lazy loading.
- Adds support for analytics configuration that use the excludes instead of includes field list.
- Adds the field used for color legends to tooltips.
@lcawl lcawl added the release_note:feature Makes this part of the condensed release notes label Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Frame Analytics ML data frame analytics features :ml release_note:enhancement release_note:feature Makes this part of the condensed release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants