Skip to content
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

Minor fixes in evals #253

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Minor fixes in evals #253

merged 4 commits into from
Dec 3, 2024

Conversation

dinmukhamedm
Copy link
Member

@dinmukhamedm dinmukhamedm commented Dec 3, 2024

  • Make target optional
  • Fix eval time progression graph
  • Fix eval scores graph

Important

This pull request includes minor fixes and enhancements in evaluation handling, graph rendering, and blog code highlighting across backend and frontend components.

  • Backend Fixes:
    • In evaluation_scores.rs, fixed SQL query condition to correctly handle bucket counts in evaluation scores.
    • Made target field optional in EvaluationDatapointResult in utils.rs.
  • Frontend Fixes:
    • In route.ts, changed default timeRange to { pastHours: 'all' } for evaluation progression.
    • Added PreHighlighter component in page.tsx for enhanced code block rendering in blog posts.
    • Adjusted blog layout in page.tsx to improve visual presentation.
  • UI Enhancements:
    • Added copy button to CodeHighlighter in code-highlighter.tsx for easier code copying.

This description was created by Ellipsis for fb50618. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to fb50618 in 18 seconds

More details
  • Looked at 179 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. app-server/src/ch/evaluation_scores.rs:220
  • Draft comment:
    Ensure that intervals.interval_num is correctly bound and used in the query to avoid logical errors. This change improves the logic to avoid counting the same value twice across different intervals.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change in app-server/src/ch/evaluation_scores.rs at line 220 introduces a condition that checks if the value equals a specific value and if the interval number matches. This is a logical improvement to avoid counting the same value twice across different intervals. However, the SQL query should ensure that the intervals.interval_num is correctly bound and used in the query to avoid logical errors.
2. frontend/app/api/projects/[projectId]/evaluation-groups/[groupId]/progression/route.ts:19
  • Draft comment:
    Verify that TimeRange supports a string value like 'all'. Changing from pastHours: 168 to pastHours: 'all' might cause issues if TimeRange does not support string values.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In frontend/app/api/projects/[projectId]/evaluation-groups/[groupId]/progression/route.ts, the change from pastHours: 168 to pastHours: 'all' at line 19 might cause issues if the TimeRange type does not support a string value. This should be verified to ensure compatibility.
3. frontend/components/blog/pre-highlighter.tsx:17
  • Draft comment:
    Add a check to ensure children.props.className is defined and contains a language class to avoid runtime errors.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In frontend/components/blog/pre-highlighter.tsx, the code assumes that children.props.className is always defined and contains a language class. This might lead to runtime errors if className is undefined or does not contain a language class. A check should be added to handle such cases gracefully.

Workflow ID: wflow_gnzGyeFZjyPqem11


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@dinmukhamedm dinmukhamedm merged commit d244713 into main Dec 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant