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] Fix DFA feature importance popover empty #91061

Merged
merged 9 commits into from
Feb 16, 2021

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Feb 10, 2021

Summary

This PR fixes #90603 which was introduced due to recent fields API changes. It also unskip related functional tests that were previously skipped here #90526

Classification

Screen Shot 2021-02-10 at 17 06 04

Regression

Screen Shot 2021-02-10 at 17 05 37

Review Hints:

  • Kibana-design team: A small change to the expandable_section.scss to fix the z-index of the Elastic charts tooltip (100) much smaller than the Eui popover z-index (8001)

Checklist

@qn895 qn895 requested a review from a team as a code owner February 10, 2021 23:13
@qn895 qn895 self-assigned this Feb 10, 2021
@qn895 qn895 added :ml Feature:Data Frame Analytics ML data frame analytics features release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0 labels Feb 10, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@qn895 qn895 changed the title [ML] Fix DFA feature importance broken cause Fields api change [ML] Fix DFA feature importance popover empty Feb 10, 2021
importance: importance[index],
})
: fi.classes,
importance: Array.isArray(fi.importance) ? fi.importance[0] : fi.importance,
Copy link
Contributor

Choose a reason for hiding this comment

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

From the type, it looks like 'importance' is a number. If it can also be an array of numbers it might be worth updating the type.

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Left a small comment but code LGTM ⚡

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.

Tested and LGTM

@peteharverson
Copy link
Contributor

Sure it isn't related to the changes here, but the tooltip is often rendering below the popover for me:

image

@qn895 qn895 requested a review from a team as a code owner February 16, 2021 17:04
// Make sure the charts tooltip in popover
// have higher zIndex than Eui popover cells
[id^='echTooltipPortal'] {
z-index: 10000;
Copy link
Contributor

Choose a reason for hiding this comment

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

This works for me only when adding !important as otherwise it gets overridden by the inline style.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Pete, this is now updated here 3f76bf8

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.

Latest edits LGTM

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Left one small SCSS comment, but LGTM otherwise. Approving now to avoid holding you up.

// Make sure the charts tooltip in popover
// have higher zIndex than Eui popover cells
[id^='echTooltipPortal'] {
z-index: 10000 !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming this achieves the same result, consider changing to an EUI z-index variable.

Suggested change
z-index: 10000 !important;
z-index: $euiZLevel9 !important;

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated here 3f25370

@qn895 qn895 added the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 16, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

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.3MB -526.0B

History

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

@qn895 qn895 merged commit 4a661cd into elastic:master Feb 16, 2021
@qn895 qn895 deleted the ml-dfa-fields-broken branch February 16, 2021 22:14
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 16, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #91577

Successful backport PRs will be merged automatically after passing CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Data Frame Analytics ML data frame analytics features :ml release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ML] Data frame analytics feature importance decision path popover broken
7 participants