Bug 2036968: Replaced fast-kde with fftkde and used bootstrap-ci to get CI summary #1034
Bug 2036968: Replaced fast-kde with fftkde and used bootstrap-ci to get CI summary #1034kala-moz wants to merge 8 commits into
Conversation
✅ 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 #1034 +/- ##
==========================================
+ Coverage 96.32% 96.37% +0.05%
==========================================
Files 113 113
Lines 3262 3254 -8
Branches 739 741 +2
==========================================
- Hits 3142 3136 -6
+ Misses 118 116 -2
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Good start! As discussed, let's switch to echarts for feature, performance and accessibility reasons. I've updated the examples in the other repo so that it is even more drop in, and added a "gallery" with multiple examples. The closest this PR looks like the examples the best it is. In particular, it's going to be a lot less visually noisy, with lots of useless numbers removed. This is a complaint I hear from perf people. lmk if we should do a quick 1:1 one off or pair to get this done! |
Thanks, @padenot. I went ahead and switched to echarts. Should I continue with the rest of the updates or should we break them into separate PRs? I feel this PR is pretty full already. |
padenot
left a comment
There was a problem hiding this comment.
As discussed, those are only some changes to do now, and we'll do the rest in a second PR. Looks nice, most comments are cosmetics or just comments (not linked to any action).
| ], | ||
| tooltip: { | ||
| trigger: 'axis', | ||
| axisPointer: { type: 'cross', crossStyle: { color: '#999' } }, |
There was a problem hiding this comment.
That's nice but I'm having issues with having the tooltip lock onto a data series, it shows the base or new but not both. Can we make it show both and lock onto the data series? E.g. a single tooltip always when overing over the KDE part, that shows base and new value at this position in x ?
|
Found a bug in my patch. The graph breaks with this comparison on some tests. https://deploy-preview-1034--mozilla-perfcompare.netlify.app/compare-results?baseRev=422aa058a4418b51b7cbccc3a6b3cc0e5f4c5d5b&baseRepo=autoland&newRev=b2144d802d0ad9cf9d259ca0cf62f9ec9b3615ec&newRepo=autoland&framework=13&sort=delta%7Cdesc |
This PR (Bug 2036968) integrates the js version of @padenot's standalone-stats into perfcompare. Here are the main changes for the integration:
Deploy Link
1.) Added
standalone-stats/kde.jsandkde.d.ts;bootstrap-ci.jsandbootstrap-ci.tstosrc/utils2.) In
src/components/CompareResults/CommonGraph.tsx- Removed the thefast-kdeimport and replaced with thefftkde. I removedapproximateSJBandwidthbecause the ISJ method below auto-selects the bandwidth per dataset. It also has an internal grid and quantile logic so didn't need thequantileSortedmethod.3.) Use the
bootstrapMedianDiffCIfrombootstrap-cito get the confidence interval summary and confidence interval statement.Note: please check that the comments are relevant as I may have missed to update or add them where necessary.
Still need to add coverage.
Updates: