[ML] Fix sampling probability when switching between automatic and manual #150389

qn895 · 2023-02-06T21:34:31Z

Summary

This PR fixes #146323 and addresses wrong initial label on sampling percentage slider. Change involves setting switching the EuiSlider to EuiSelect with 100% as the a new option. This means when switching from On - automatic or Off (where both are at 100%), switching to On - manual will by default switch to 0.001%.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests 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)
This was checked for cross-browser compatibility

elasticmachine · 2023-02-06T21:34:35Z

Pinging @elastic/ml-ui (:ml)

pheyos

Tested locally and could still produce the result described in #146323 with slightly extended steps:

Open an index in the data visualizer
In the random sampler popover switch to On - manual
- here was the original issue and this is now working fine: displays 50% / 50
Select a different percentage, e.g. 25%
Switch the random sampling to Off
Switch the random sampling to On - manual again
- => the percentage is set to 50% and the label displays 100

This reverts commit 7b2b7e9

peteharverson · 2023-02-08T17:32:04Z

Some early feedback on using the range slider with the combined input - I like the potential the input provides for allowing more fine-grained control at the lower end of the scale (% less than 5).

Looks like the input currently only allows a step size of 1 so it isn't possible to e.g. edit from 1 to 0.01.
Edit the behavior so that an edit is only applied to the view either when the popover is closed, or by adding a 'Apply' button. Currently the view does a lot of unnecessary reloads as you move the slider or input up and down in value.

qn895 · 2023-02-09T00:00:22Z

Some early feedback on using the range slider with the combined input - I like the potential the input provides for allowing more fine-grained control at the lower end of the scale (% less than 5).

Thanks for the feedback. I have updated the slider & numerical input to allow any value between 0.001% to 50%. I also added an Apply button so that the value would only be updated when clicked. Once applied, switching to another mode and then back would restore the previously applied value.

Screen.Recording.2023-02-08.at.17.56.37.mov

Otherwise, for the first time switching from Off or On - automatic (where probability automatically set at 100%) to On - manual, it will default to 0.001%.

peteharverson · 2023-02-09T11:34:36Z

.../public/application/common/components/document_count_content/random_sampler_range_slider.tsx

+  const isInvalidSamplingProbabilityInput =
+    !isDefined(samplingProbabilityInput) ||
+    isNaN(samplingProbabilityInput) ||
+    samplingProbabilityInput <= 0 ||


Should the check here be <= 0.001? Currently you can enter a value less than this and see the red border on the input but the button is still enabled.

Thanks for catching that. Fixed here 9dcd987 (#150389)

peteharverson · 2023-02-09T11:39:32Z

.../public/application/common/components/document_count_content/random_sampler_range_slider.tsx

+              const prevValue = samplingProbabilityInput ? samplingProbabilityInput * 100 : value;
+
+              if (value > 0 && value <= 1) {
+                setSamplingProbabilityInput(value / 100);


I see occasional issues with the rounding here when using the up / down buttons. Not sure if this can be fixed by our code here?

Here I started at 0.01:

Thanks for catching that. Updated here 9dcd987 (#150389)

peteharverson

Overall looks good. Left a couple of comments.

peteharverson

Tested latest changes and LGTM.

pheyos · 2023-02-10T12:25:27Z

Tested again locally on the latest changes and with the new control to select the sampling percentage, we don't have that originally raised label issue anymore. But I spotted another weird behavior where the marker position on the slider doesn't match the axis labeling when setting the value by clicking the axis / axis labels, while it's working fine when using the up/down buttons:

qn895 · 2023-02-14T02:34:45Z

@pheyos Thanks for catching that. Looks like it was an issue with the EUI show ticks & inputWithPopover setting. I've updated it to show the range on the two sides instead.

pheyos · 2023-02-14T10:18:44Z

@qn895 Tested the latest changes locally and the above mentioned issue didn't occur anymore.
However, I have two follow-up usability observations:

What is the grey bar above the slider? To me it doesn't seem to do anything and can be a bit distracting.
Slowly dragging the slider causes flickering on the slider label and "mini-resets" of slider values (e.g. slowly dragging it from 25 to 26 sometimes causes to show up the 26 shortly, then it's reset to 25).

Not sure if this is something we want to address at all and if we want to address it as part of this PR.

qn895 · 2023-02-14T15:01:43Z

@pheyos This seems to be an issue with EUI. If it's not a blocker, I think we can address this later.

walterra

LGTM, left just a small suggestion on possibly restructuring component nesting.

walterra · 2023-02-14T15:11:13Z

...lizer/public/application/common/components/document_count_content/document_count_content.tsx

@@ -139,6 +118,18 @@ export const DocumentCountContent: FC<Props> = ({
      </div>
    ) : null;

+  const CalculatingProbMessage =


Nit: Suggest to make ProbabilityUsedMessage and CalculatingProbMessage React components on the outer level and not within that component.

Updated here 1d0e2a6 (#150389)

pheyos

LGTM
As discussed above, we can look into the slider behavior separately.

kibana-ci · 2023-02-14T17:09:35Z

💚 Build Succeeded

Buildkite Build
Commit: 1d0e2a6

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dataVisualizer`	303	304	+1

Async chunks

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

id	before	after	diff
`dataVisualizer`	379.1KB	380.4KB	+1.3KB
`ml`	3.5MB	3.5MB	+38.0B
total			+1.3KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`dataVisualizer`	33	32	-1

Total ESLint disabled count

id	before	after	diff
`dataVisualizer`	33	32	-1

History

💚 Build #107793 succeeded 336e266
💚 Build #106946 succeeded 6f59538
💔 Build #106635 failed a9a8eb2
💔 Build #106304 failed d83064d
💔 Build #106226 failed 1d55dd0
💛 Build #105896 was flaky 1a45f6a

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

cc @qn895

…nual (elastic#150389) (cherry picked from commit 7fe8154)

kibanamachine · 2023-02-14T17:15:10Z

💚 All backports created successfully

Status	Branch	Result
✅	8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…and manual (#150389) (#151182) # Backport This will backport the following commits from `main` to `8.7`: - [[ML] Fix sampling probability when switching between automatic and manual (#150389)](#150389)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Quynh Nguyen (Quinn) <43350163+qn895@users.noreply.github.com>

…nual (elastic#150389)

Fix sampling probability when switching between automatic and manual

1a45f6a

qn895 added release_note:fix :ml Feature:File and Index Data Viz ML file and index data visualizer v8.7.0 labels Feb 6, 2023

qn895 requested review from pheyos and peteharverson February 6, 2023 21:34

qn895 requested a review from a team as a code owner February 6, 2023 21:34

qn895 self-assigned this Feb 6, 2023

pheyos reviewed Feb 7, 2023

View reviewed changes

qn895 added 4 commits February 7, 2023 12:56

Switch to select

7b2b7e9

Merge remote-tracking branch 'upstream/main' into dv-fix-sampling-rate

1d55dd0

Revert "Switch to select"

b595ef5

This reverts commit 7b2b7e9

Switch with inputWithPopover

d83064d

peteharverson changed the title ~~Fix sampling probability when switching between automatic and manual~~ [ML] Fix sampling probability when switching between automatic and manual Feb 8, 2023

qn895 added 3 commits February 8, 2023 14:42

Fix behavior of input not increasing correctly, add apply button

189a590

Fix behavior of input not increasing correctly, add apply button

a9a8eb2

Clean up

9c2713f

peteharverson reviewed Feb 9, 2023

View reviewed changes

qn895 added 2 commits February 9, 2023 09:21

Fix logic for disabled button

9dcd987

Merge remote-tracking branch 'upstream/main' into dv-fix-sampling-rate

6f59538

qn895 requested a review from walterra February 9, 2023 15:25

peteharverson approved these changes Feb 9, 2023

View reviewed changes

Coerce 'true' & 'false' to match 'True' & 'False'

baacd17

qn895 added 2 commits February 13, 2023 19:52

Merge remote-tracking branch 'upstream/main' into dv-fix-sampling-rate

340cbad

Fix range

336e266

walterra approved these changes Feb 14, 2023

View reviewed changes

Refactor ProbabilityUsedMessage and CalculatingProbabilityMessage

1d0e2a6

qn895 enabled auto-merge (squash) February 14, 2023 16:28

pheyos approved these changes Feb 14, 2023

View reviewed changes

qn895 merged commit 7fe8154 into elastic:main Feb 14, 2023

kibanamachine added the v8.8.0 label Feb 14, 2023

kibanamachine mentioned this pull request Feb 14, 2023

[8.7] [ML] Fix sampling probability when switching between automatic and manual (#150389) #151182

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 14, 2023

[ML] Fix sampling probability when switching between automatic and ma…

84cc048

…nual (elastic#150389) (cherry picked from commit 7fe8154)

justinkambic pushed a commit to justinkambic/kibana that referenced this pull request Feb 23, 2023

[ML] Fix sampling probability when switching between automatic and ma…

90ec25b

…nual (elastic#150389)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Fix sampling probability when switching between automatic and manual #150389

[ML] Fix sampling probability when switching between automatic and manual #150389

qn895 commented Feb 6, 2023 •

edited by kibanamachine

Loading

elasticmachine commented Feb 6, 2023

pheyos left a comment

peteharverson commented Feb 8, 2023

qn895 commented Feb 9, 2023

peteharverson Feb 9, 2023

qn895 Feb 9, 2023

peteharverson Feb 9, 2023

qn895 Feb 9, 2023

peteharverson left a comment

peteharverson left a comment

pheyos commented Feb 10, 2023

qn895 commented Feb 14, 2023

pheyos commented Feb 14, 2023

qn895 commented Feb 14, 2023

walterra left a comment

walterra Feb 14, 2023

qn895 Feb 14, 2023

pheyos left a comment

kibana-ci commented Feb 14, 2023

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Feb 14, 2023

[ML] Fix sampling probability when switching between automatic and manual #150389

[ML] Fix sampling probability when switching between automatic and manual #150389

Conversation

qn895 commented Feb 6, 2023 • edited by kibanamachine Loading

Summary

Checklist

elasticmachine commented Feb 6, 2023

pheyos left a comment

Choose a reason for hiding this comment

peteharverson commented Feb 8, 2023

qn895 commented Feb 9, 2023

peteharverson Feb 9, 2023

Choose a reason for hiding this comment

qn895 Feb 9, 2023

Choose a reason for hiding this comment

peteharverson Feb 9, 2023

Choose a reason for hiding this comment

qn895 Feb 9, 2023

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

pheyos commented Feb 10, 2023

qn895 commented Feb 14, 2023

pheyos commented Feb 14, 2023

qn895 commented Feb 14, 2023

walterra left a comment

Choose a reason for hiding this comment

walterra Feb 14, 2023

Choose a reason for hiding this comment

qn895 Feb 14, 2023

Choose a reason for hiding this comment

pheyos left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 14, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Feb 14, 2023

💚 All backports created successfully

Questions ?

qn895 commented Feb 6, 2023 •

edited by kibanamachine

Loading