feat: overall author rankings in leaderboard #126
Conversation
…ew in author rankings tab.
WalkthroughThis change introduces per-author speedup metrics calculated via area-based integration of performance curves. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @YiyanZhai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the leaderboard by introducing a new 'Author rankings' tab. This tab provides an overall ranking of authors based on their cumulative speedup, calculated as the area under their Fast P curve across all workloads. This new metric offers a more holistic view of author performance, complementing the existing correctness view by highlighting consistent speed improvements. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new overall author ranking feature to the leaderboard, calculated as the area under the Fast-P curve. The changes span data computation in lib/analytics.ts and UI presentation in app/leaderboard/client.tsx. The implementation is solid, correctly calculating the area and displaying the new rankings. My feedback includes a suggestion to refactor a data transformation to a more modern functional style for better maintainability, and a fix for a duplicated UI component that appears in two different tabs.
| <OverallRankingCard | ||
| rankings={overallRanking} | ||
| maxTotalSpeedup={maxTotalSpeedup} | ||
| formatCoverage={formatCoverage} | ||
| title="Overall author rankings" | ||
| description="Area under the Fast-p curve across every workload; higher values mean more consistent speedups over the baseline." | ||
| /> |
There was a problem hiding this comment.
The OverallRankingCard component is rendered in both the "Fast-p" tab and the "Author rankings" tab. This seems redundant. Based on the PR description, this new ranking card is intended for the author rankings view. To avoid duplication and keep the UI clean, it should probably only be displayed in the "Author rankings" tab.
| const overallRanking = useMemo<OverallAuthorRanking[]>(() => { | ||
| const entries: OverallAuthorRanking[] = [] | ||
| const speedupEntries = Object.entries(fast.speedupSums ?? {}) | ||
| for (const [author, totalSpeedup] of speedupEntries) { | ||
| if (excludedSet.has(author)) continue | ||
| const comparisons = fast.comparisonCounts[author] ?? 0 | ||
| if (comparisons <= 0) continue | ||
| entries.push({ | ||
| author, | ||
| totalSpeedup, | ||
| comparisons, | ||
| coverage: fast.coverage?.[author], | ||
| correctness: correctnessByAuthor[author], | ||
| }) | ||
| } | ||
| return entries.sort((a, b) => { | ||
| if (b.totalSpeedup !== a.totalSpeedup) return b.totalSpeedup - a.totalSpeedup | ||
| if (b.comparisons !== a.comparisons) return b.comparisons - a.comparisons | ||
| return a.author.localeCompare(b.author) | ||
| }) | ||
| }, [correctnessByAuthor, excludedSet, fast.comparisonCounts, fast.coverage, fast.speedupSums]) |
There was a problem hiding this comment.
The overallRanking calculation can be rewritten using a more functional and declarative style with array methods like map, filter, and sort. This improves readability and avoids mutable state by removing the intermediate entries array.
const overallRanking = useMemo<OverallAuthorRanking[]>(() => {
return Object.entries(fast.speedupSums ?? {})
.map(([author, totalSpeedup]) => ({
author,
totalSpeedup,
comparisons: fast.comparisonCounts[author] ?? 0,
coverage: fast.coverage?.[author],
correctness: correctnessByAuthor[author],
}))
.filter((entry) => !excludedSet.has(entry.author) && entry.comparisons > 0)
.sort((a, b) => {
if (b.totalSpeedup !== a.totalSpeedup) return b.totalSpeedup - a.totalSpeedup;
if (b.comparisons !== a.comparisons) return b.comparisons - a.comparisons;
return a.author.localeCompare(b.author);
});
}, [correctnessByAuthor, excludedSet, fast.comparisonCounts, fast.coverage, fast.speedupSums])
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/apps/web/app/leaderboard/section.tsx (1)
25-34: Refactor: Extract shared type definition.The
DefinitionAuthorDetailtype is defined in both this file (line 25) and inweb/apps/web/app/leaderboard/client.tsx(line 20). Duplicate type definitions can lead to maintenance issues if the definitions diverge.Consider extracting this type to a shared location, such as
@/lib/analyticsor a new@/app/leaderboard/types.tsfile, and importing it in both files.
🧹 Nitpick comments (1)
web/apps/web/lib/analytics.ts (1)
298-331: LGTM: Area-under-curve calculation is mathematically sound.The trapezoidal integration correctly computes the area under each author's Fast-p curve. The division by 200 appropriately averages the two percent values and converts from percentage to decimal.
Consider adding a brief comment explaining the integration formula for future maintainers:
curves[author] = points let area = 0 + // Trapezoidal integration: sum of (width × average_height) for each segment for (let index = 1; index < points.length; index++) { const prev = points[index - 1] const current = points[index] const width = current.p - prev.p if (width <= 0) continue - const height = (prev.percent + current.percent) / 200 + const height = (prev.percent + current.percent) / 200 // Average percent as decimal area += width * height } speedupSums[author] = areaAlso applies to: 342-342
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/apps/web/app/leaderboard/client.tsx(8 hunks)web/apps/web/app/leaderboard/section.tsx(3 hunks)web/apps/web/lib/analytics.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
web/apps/web/app/leaderboard/client.tsx (2)
web/apps/web/lib/analytics.ts (2)
CoverageStats(35-39)CorrectnessSummary(7-13)web/apps/web/components/fast-p-label.tsx (1)
FastPLabel(8-19)
web/apps/web/app/leaderboard/section.tsx (1)
web/apps/web/lib/analytics.ts (1)
computeFastPCurvesForAuthors(237-343)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run unit tests on ubuntu-latest and Python 3.11
- GitHub Check: Run unit tests on ubuntu-latest and Python 3.10
- GitHub Check: Run unit tests on ubuntu-latest and Python 3.13
- GitHub Check: Run unit tests on ubuntu-latest and Python 3.12
🔇 Additional comments (6)
web/apps/web/lib/analytics.ts (1)
27-27: LGTM: speedupSums field addition.The new field correctly extends AuthorCurvesResponse to expose per-author cumulative speedup metrics.
web/apps/web/app/leaderboard/section.tsx (1)
67-67: LGTM: speedupSums propagation.The destructuring and inclusion of
speedupSumscorrectly threads the new field through the data flow.Also applies to: 93-93
web/apps/web/app/leaderboard/client.tsx (4)
40-46: LGTM: OverallAuthorRanking type definition.The type appropriately models the aggregated ranking data with optional coverage and correctness fields.
304-338: LGTM: Ranking computation logic is sound.The
correctnessByAuthormap andoverallRankingcomputation correctly:
- Build a lookup structure for per-author correctness
- Filter excluded authors and zero-comparison entries
- Sort by speedup (descending), then comparisons (descending), then author name (ascending)
611-663: LGTM: OverallRankingCard implementation.The component correctly:
- Normalizes bar widths relative to
maxTotalSpeedup- Displays area value with 2 decimal places
- Shows supplementary metrics (comparisons, coverage, pass rate)
- Handles edge cases (no rankings, missing data)
98-98: LGTM: Tab restructure from "correctness" to "rankings".The tab changes appropriately consolidate both speedup-based and correctness-based rankings under a unified "rankings" tab, improving the UI organization.
Also applies to: 371-371, 378-378, 541-548
1 participant
This PR adds cumulative speedup totals to the original correctness view in author rankings tab.
The overall author ranking is calculated using the area under each author's Fast P curve.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.