-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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: Highlight filtered data in scatterplot charts #144871
[ML] Data Frame Analytics: Highlight filtered data in scatterplot charts #144871
Conversation
4f11e56
to
209d813
Compare
…-ref HEAD~1..HEAD --fix'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great feature, I like how you've achieved this with Vega layers! There's now quite some redundant code though. Suggest to consolidate the layer code into a function where you can pass in options to get a spec for foreground and background layer.
I played around a bit with what Pete suggested regarding making the unselected dots part of the legend. IMHO this is a bit messy, Vega tries to combine the colors based on different attributes into one, will be even more problematic for classification/regression.
That legend item is also in conflict with brush selecting where the style chart with gray unselected areas will be the same but no legend will indicate that.
An alternative could be an explanatory text when there's a background distribution shown below the chart or on an info-icon somewhere.
(Just for reference the stuff I changed to get that adapted but not so great working legend: scatterplot_matrix_vega_lite_spec.ts.zip)
function filterChartableItems(items: estypes.SearchHit[], resultsField?: string) { | ||
return ( | ||
items | ||
?.map((d) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need the ?
here since items
isn't optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - good catch - updated in 1ed0db8
}; | ||
|
||
if (backgroundValues.length) { | ||
schema.spec.layer.push({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest to use unshift
here instead of push
to add the background layer under the regular items otherwise the gray dots will be drawn on top of the foreground data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Updated in 1ed0db8
This sounds a good alternative to indicating it in the legend. Maybe below the chart? I can't think of any obvious place to add an info icon. |
Pinging @elastic/ml-ui (:ml) |
Added helper text and consolidated layer code into a function to prevent duplication in 1ed0db8 This is ready for a final look when you get a chance - cc @walterra, @peteharverson 🙏 |
@elasticmachine merge upstream |
cc @lcawl regarding the help text below the charts |
…-ref HEAD~1..HEAD --fix'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest changes LGTM!
fullWidth | ||
helpText={i18n.translate('xpack.ml.splom.backgroundLayerHelpText', { | ||
defaultMessage: | ||
"If the data points match your filter, they're shown in color; otherwise, they're blurred gray", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: Might deserve a dot at the end of the sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 6047a9e
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Agree with @walterra about the minor text edit for the label about chart colors with a filter.
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
…rts (elastic#144871) ## Summary Related meta issue: elastic#131551 This PR adds functionality to the scatterplot charts to show the full data sample and, when the user has added a filter/query in the query bar, the portion of the data reflecting the filter is highlighted so it can be differentiated from the background data. Classification results view with query for `AvgTicketPrice > 400` <img width="1032" alt="image" src="https://user-images.githubusercontent.com/6446462/200716771-b2012e9b-c620-46a8-9dc3-92df23ef4476.png"> Outlier detection results view with same filter <img width="1026" alt="image" src="https://user-images.githubusercontent.com/6446462/200716858-01407906-34de-43d6-892b-7bbfede05eac.png"> Regression results view with same filter <img width="1007" alt="image" src="https://user-images.githubusercontent.com/6446462/200716910-41165b81-a300-420c-8976-47a0ea9612bf.png"> Help text: <img width="1005" alt="image" src="https://user-images.githubusercontent.com/6446462/201484563-9f4ca87b-3025-485f-ac0e-4a30deee847f.png"> ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
Related meta issue: #131551
This PR adds functionality to the scatterplot charts to show the full data sample and, when the user has added a filter/query in the query bar, the portion of the data reflecting the filter is highlighted so it can be differentiated from the background data.
Classification results view with query for
AvgTicketPrice > 400
Outlier detection results view with same filter
Regression results view with same filter
Help text:
Checklist
Delete any items that are not applicable to this PR.