Bug: 2044194 Port client-side KDE mode detection from kde-widget into CommonGraph#1045
Bug: 2044194 Port client-side KDE mode detection from kde-widget into CommonGraph#1045kala-moz wants to merge 3 commits into
Conversation
…, mode-line overlays, peak labels (ported from kde-widget)
✅ Deploy Preview for mozilla-perfcompare ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1045 +/- ##
==========================================
+ Coverage 96.19% 96.34% +0.14%
==========================================
Files 113 113
Lines 3311 3364 +53
Branches 760 772 +12
==========================================
+ Hits 3185 3241 +56
+ Misses 125 122 -3
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kala-moz
changed the title
padenot
left a comment
padenot
left a comment
There was a problem hiding this comment.
Some comments and one feature request from Denis on the perf team.
| > | ||
| {Math.round(vt * 100)}% | ||
| </Typography> | ||
| </Box> |
There was a problem hiding this comment.
I've been asked for a simple checkbox that disables the mode detection overlay, it can help in some cases. Just a checkbox that hides it.
| ], | ||
| }; | ||
| }, [baseValues, newValues, unit, isSubtest]); | ||
| }, [baseValues, newValues, unit, isSubtest, vt]); |
There was a problem hiding this comment.
this causes constant rerenders when changing the slider, causing a change in the scatter. We should memory this new vt on its own, because we don't want to recompute everything when it changes, just the modal analysis.
| * ub : base indices with no match (path disappeared) | ||
| * un : new indices with no match (new path appeared) | ||
| */ | ||
| export function matchModes(bLocs, bFracs, nLocs, nFracs) { |
There was a problem hiding this comment.
not used yet, I think used later?
| unit={baseUnit || newUnit} | ||
| isSubtest={result.base_parent_signature !== null} | ||
| vt={vt} | ||
| onVtChange={setVt} |
There was a problem hiding this comment.
is this debounced? we all have fast computers but it should probably be debounced
| : { peakLocs: [], fracs: [], letters: [] }; | ||
| const newModes = nKde | ||
| ? computeModeInfo(sharedX, newY, vt) | ||
| : { peakLocs: [], fracs: [], letters: [] }; |
There was a problem hiding this comment.
We shouldn't be doing this should we? This isn't related to rendering, and can be expensive. Can we move it out, so it's not recomputed frequently?
2 participants
See Bug 2044194
This PR adds client-side KDE mode detection by porting the pure-logic helpers (
fitModesFromKde, areaFracs, assignLetters, matchModes, splitByMode) from thekde-widgetintosrc/utils/kde.js. See Paul's example here.It wires those helpers into
CommonGraphso detected peaks render as labelled vertical markers on the KDE chart (e.g. "Base A: 12.34 (45%)"). Also adds the interactive valley-depth slider — with the threshold state lifted toRevisionRowExpandableso the upcoming mode-blurb panel can read the same modes without recomputing the KDE.Includes unit tests for the new pure functions and updated CommonGraph tests covering the slider props and mode-overlay series.
Note: There's still more work to do like the upcoming blurb panel but didn't want to overload this PR by touching too many different files.
Deploy Link