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.

Sign up for GitHub

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

Jump to bottom

[ML] DF Analytics results view: ensure boolean values in charts shown without formatting #78888

Merged
merged 4 commits into from
Oct 6, 2020
Merged

[ML] DF Analytics results view: ensure boolean values in charts shown without formatting #78888

merged 4 commits into from
Oct 6, 2020

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Sep 29, 2020 •
edited
Loading

Summary

Related meta issue: #77182

  • update Chart data items to always have key_as_string property and use them in the charts so that values like booleans show up as 'true'/'false' vs 1/0

image

  • Adds functional tests for training filters to ensure training filter exists in relevant views.

Checklist

Delete any items that are not applicable to this PR.

@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner September 29, 2020 23:28
@alvarezmelissa87 alvarezmelissa87 self-assigned this Sep 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Sep 29, 2020
Closed
5 tasks
@peteharverson peteharverson changed the title [ML] DF Analytics results view: ensure booelan values shown without formatting [ML] DF Analytics results view: ensure boolean values shown without formatting Sep 30, 2020
@peteharverson
Copy link
Contributor

This looks good. But as part of this work would be nice to look at the behavior of the tooltip for the data grid histogram for boolean types to also show true / false rather than 1 / 0 currently:

image

@walterra suggested the easiest approach might be to adapt the data we pass on to the chart instead of fiddling with custom formatting in the tooltip.

@peteharverson
Copy link
Contributor

peteharverson commented Sep 30, 2020 •
edited by alvarezmelissa87
Loading

Regarding #78888 (comment), it looks like the schema change has changed the axis labelling for boolean types. On master, it labels the columns as 'true' / 'false' rather than '2 categories' (although the tooltip still says 1 / 0):

image

@peteharverson - thanks for taking a look! 🙏 I was able to fix the tooltip issue but it requires keeping the schema for boolean defined - which causes the boolean values to be displayed as capitalized. Looks like those are coupled for now - I'll do some more digging.

@alvarezmelissa87 alvarezmelissa87 changed the title [ML] DF Analytics results view: ensure boolean values shown without formatting [ML] DF Analytics results view: ensure boolean values in charts shown without formatting Sep 30, 2020
pheyos
pheyos approved these changes Oct 2, 2020
View reviewed changes
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 test changes LGTM

jgowdyelastic
jgowdyelastic reviewed Oct 6, 2020
View reviewed changes
x-pack/plugins/ml/public/application/components/data_grid/use_column_chart.tsx Outdated
@@ -231,11 +231,13 @@ export const useColumnChart = (
if (isOrdinalChartData(chartData)) {
data = chartData.data.map((d: OrdinalDataItem) => ({
...d,
key_as_string: d.key_as_string || d.key,
Copy link
Member

Choose a reason for hiding this comment

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

if d.key_as_string could be undefined this should probably be a ?? check.
also, should key_as_string be optional in the interface?

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Oct 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, d.key_as_string is not defined in some items this point so we are checking for undefined. Great suggestion for updating to use ?? check.

As key_as_string will now always be defined (since we're setting it as key when it isn't) I left it as required in the interface. Though it will be less flexible and doesn't really reflect the original item - I think leaving it as optional is the better option. Good catch! 🙏

Updated in df3142a

peteharverson
peteharverson approved these changes Oct 6, 2020
View reviewed changes
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 LGTM.

Formatting true in the table cell rather than True might be best done eventually in the EUI data grid itself.

jgowdyelastic
jgowdyelastic approved these changes Oct 6, 2020
View reviewed changes
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
ml 10.6MB 10.6MB +696.0B

History

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

@alvarezmelissa87 alvarezmelissa87 merged commit 4c65b6d into elastic:master Oct 6, 2020
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Oct 6, 2020
@alvarezmelissa87
[ML] DF Analytics results view: ensure boolean values in charts shown…
5a8a5c5 
… without formatting (elastic#78888)

* add functional test for searchBar filters. remove boolean schema from datagrid

* always use string version of key for charts

* add schema back in for histogram label

* use ?? instead of || for undefined check
@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Oct 6, 2020
Merged
@alvarezmelissa87 alvarezmelissa87 deleted the ml-dfanalytics-update branch October 6, 2020 18:41
alvarezmelissa87 added a commit that referenced this pull request Oct 6, 2020
@alvarezmelissa87
[ML] DF Analytics results view: ensure boolean values in charts shown…
cd7f5dd 
… without formatting (#78888) (#79741)

* add functional test for searchBar filters. remove boolean schema from datagrid

* always use string version of key for charts

* add schema back in for histogram label

* use ?? instead of || for undefined check
tylersmalley pushed a commit to tylersmalley/kibana that referenced this pull request Oct 7, 2020
Revert "[ML] DF Analytics results view: ensure boolean values in char…
761bbb3 
…ts shown without formatting (elastic#78888)"

This reverts commit 4c65b6d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgowdyelastic jgowdyelastic jgowdyelastic approved these changes

@peteharverson peteharverson peteharverson approved these changes

@pheyos pheyos pheyos approved these changes

@walterra walterra Awaiting requested review from walterra

Assignees

@alvarezmelissa87 alvarezmelissa87

Labels
Feature:Data Frame Analytics ML data frame analytics features :ml release_note:enhancement v7.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alvarezmelissa87 @elasticmachine @peteharverson @kibanamachine @pheyos @jgowdyelastic