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: ROC Curve Chart #89991

Merged
merged 27 commits into from
Feb 15, 2021
Merged

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Feb 2, 2021

Summary

Part of #90421.

Adds the ROC curve chart to the results page for classification jobs in the evaluate section.

Review hints:

  • SCSS/Kibana UI Team: Only a SCSS file and one CSS class was renamed. For better aligning the confusion matrix and ROC curve chart a percentage based width was changed to a fixed width.
  • The large diff related to the Scatterplot Matrix component was caused by a rename again.
  • The scatterplot matrix and ROC curve chart now share an underlying VegaChart component that takes care of lazy loading.
  • Loading data for the evaluate panel was split up into custom hooks.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@walterra walterra self-assigned this Feb 2, 2021
@walterra walterra changed the title [ML] Data Frame Analytics: AUC ROC Chart [ML] Data Frame Analytics: ROC Curve Chart Feb 4, 2021
@walterra walterra marked this pull request as ready for review February 5, 2021 12:05
@walterra walterra requested review from a team as code owners February 5, 2021 12:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@peteharverson
Copy link
Contributor

peteharverson commented Feb 5, 2021

Anything we can do to improve the behavior here, when trying to query for class. Could also add a padding around the callout.

Would it make more sense not to use the query at all?

image

@walterra
Copy link
Contributor Author

walterra commented Feb 9, 2021

@peteharverson Improved the error handling:

  • All error will now be displayed within one error callout.
  • The header of the ROC Curve Chart will always be present so the context of the error messages is more clear.
  • The "other" column will now be ignored and not cause an error.

image

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 Feb 9, 2021

LGTM

…ices from specific scatterplot service to more generic CanvasElement service.
@walterra walterra requested a review from pheyos February 9, 2021 16:18
@walterra
Copy link
Contributor Author

walterra commented Feb 9, 2021

@pheyos Updated functional tests to assert new RocCurveChart. Refactored services from specific scatterplot service to a more generic CanvasElement service that's now used for both ScatterplotMatrix and RocCurveChart.

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.

Already discussed on slack, but repeating here for visibility:
I've noticed that the confusion matrix is now smaller than it used to be.
Before this PR, I could e.g. view the full matrix for the iris job:
image
But now the width seems to be aligned to the ROC curve, so I have to scroll around even in this small matrix to view all values:
image

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 🎉

@walterra
Copy link
Contributor Author

Investigating what's wrong with the bundle size increase with +7.4MB. A local analysis with webpack-bundle-analyzer shows only a few KB difference between master (6.93MB) and this PR (6.94MB).

@walterra
Copy link
Contributor Author

@pheyos Fixed confusion matrix width in 98cfdb6.

@walterra
Copy link
Contributor Author

@pheyos the issue with the scrollbars appearing for the confusion matrix should be fixed again:

image

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1735 1742 +7

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.3MB 6.4MB +7.0KB

History

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

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.

LGTM

@walterra walterra merged commit 2f845dd into elastic:master Feb 15, 2021
@walterra walterra deleted the ml-auc-roc branch February 15, 2021 13:49
walterra added a commit to walterra/kibana that referenced this pull request Feb 15, 2021
Adds the ROC curve chart to the results page for classification jobs in the evaluate section.
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 15, 2021
…ndition-for-hiding-recommded-allocation

* 'master' of github.com:elastic/kibana:
  [Discover] Fix toggling multi fields from doc view table (elastic#91121)
  [ML] Data Frame Analytics: ROC Curve Chart (elastic#89991)
  skip flaky suite (elastic#86948)
  skip flaky suite (elastic#91191)
  Fix date histogram time zone for rollup index (elastic#90632)
  [Search Source] Fix retrieval of unmapped fields; Add field filters (elastic#89837)
  [Logs UI] Use useMlHref hook for ML links (elastic#90935)
  Fix values of `products.min_price` field in Kibana sample ecommerce data set (elastic#90428)
  [APM] Darker shade for Error group details labels (elastic#91349)
  [Lens] Adjust new copy for 7.12 (elastic#90413)
  [ML] Unskip test. Fix modelMemoryLimit value. (elastic#91280)
  [Lens] Fix empty display name issue in XY chart (elastic#91132)
  [Lens] Improves error messages when in Dashboard (elastic#90668)
  [Lens] Keyboard-selected items follow user traversal of drop zones (elastic#90546)
  [Lens] Improves ranking feature in Top values (elastic#90749)
  [ILM] Rollover min age tooltip and copy fixes (elastic#91110)

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts
walterra added a commit that referenced this pull request Feb 15, 2021
Adds the ROC curve chart to the results page for classification jobs in the evaluate section.
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.

7 participants