-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix: prevent negative cost values and improve label visibility in evals chart #7830
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
Conversation
…lity in evals chart
|
🚀 Preview deployed! Your changes have been deployed to Vercel: Preview URL: https://roo-code-website-gkcg5sa1n-roo-code.vercel.app This preview will be updated automatically when you push new commits to this PR. |
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.
Reviewing my own code because apparently I trust no one, not even myself.
| name="Cost" | ||
| domain={[ | ||
| (dataMin: number) => Math.round((dataMin - 5) / 5) * 5, | ||
| (dataMin: number) => Math.max(0, Math.round((dataMin - 5) / 5) * 5), |
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 fix for preventing negative costs! I notice the Y-axis already uses this same pattern on line 194, so this improves consistency.
One thought: could we also consider adding a small right margin (maybe 10-20px) to prevent potential label truncation on the right edge, similar to what we did for the top margin?
| domain={[ | ||
| (dataMin: number) => Math.round((dataMin - 5) / 5) * 5, | ||
| (dataMin: number) => Math.max(0, Math.round((dataMin - 5) / 5) * 5), | ||
| (dataMax: number) => Math.round((dataMax + 5) / 5) * 5, |
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.
The value 5 appears to be used as a rounding factor here and in other domain calculations. Would it make sense to extract this to a named constant like AXIS_ROUNDING_FACTOR or add a comment explaining why 5 was chosen? This would make the intent clearer for future maintenance.
This PR fixes two issues with the evals chart on the website:
Issues Fixed
Negative cost values: The X-axis domain calculation could result in negative values when the minimum cost was less than $5. Added
Math.max(0, ...)to ensure the cost axis always starts at $0 or higher.Truncated labels: Labels for data points at the top of the chart were being cut off due to insufficient top margin. Increased the top margin from 0 to 20 pixels to provide adequate space for labels.
Changes Made
apps/web-roo-code/src/app/evals/plot.tsx:Testing
The changes have been verified to:
Fixes requested via Slack mention.
Important
Fixes negative cost values and improves label visibility in
evals/plot.tsxby adjusting X-axis domain and chart margin.plot.tsxby usingMath.max(0, ...).plot.tsxto prevent label truncation.This description was created by
for 92efe55. You can customize this summary. It will automatically update as commits are pushed.