Skip to content

Conversation

PatKamin
Copy link
Contributor

Improve readability of Historical Results chart titles, tooltips, and all charts' legends by introducing new Result object field: display_name for Compute Benchmarks

Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

Here's an idea that might simplify this patch:
move display_name() to be a benchmark metadata field. That we we never have to update old results, and we can just look it up in metadata in the scripts if needed.

@PatKamin
Copy link
Contributor Author

Here's an idea that might simplify this patch: move display_name() to be a benchmark metadata field. That we we never have to update old results, and we can just look it up in metadata in the scripts if needed.

Thanks, made changes

@PatKamin PatKamin force-pushed the add-display-name branch from 32ee7f1 to fca7432 Compare May 21, 2025 07:24
@PatKamin PatKamin force-pushed the add-display-name branch from fca7432 to 7130ac8 Compare May 21, 2025 07:40
@PatKamin PatKamin temporarily deployed to WindowsCILock May 21, 2025 07:41 — with GitHub Actions Inactive
@PatKamin PatKamin temporarily deployed to WindowsCILock May 21, 2025 07:59 — with GitHub Actions Inactive
@PatKamin PatKamin temporarily deployed to WindowsCILock May 21, 2025 07:59 — with GitHub Actions Inactive
@PatKamin PatKamin force-pushed the add-display-name branch from 7130ac8 to 1721202 Compare May 22, 2025 13:07
@PatKamin PatKamin temporarily deployed to WindowsCILock May 22, 2025 13:07 — with GitHub Actions Inactive
@PatKamin PatKamin marked this pull request as ready for review May 22, 2025 13:08
@PatKamin PatKamin requested a review from a team as a code owner May 22, 2025 13:08
@PatKamin PatKamin requested a review from pbalcer May 22, 2025 13:08
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

mostly lgtm, I like this approach better. I'm not sure what we can do with CPU Count stuff, but I really don't like fixing the metadata after the benchmarks ran.
If we can't figure out a better approach, we should try to isolate this hack better.

@PatKamin PatKamin temporarily deployed to WindowsCILock May 22, 2025 13:30 — with GitHub Actions Inactive
@PatKamin PatKamin temporarily deployed to WindowsCILock May 22, 2025 13:30 — with GitHub Actions Inactive
@PatKamin PatKamin force-pushed the add-display-name branch from 1721202 to d9091a9 Compare May 23, 2025 08:53
@PatKamin PatKamin temporarily deployed to WindowsCILock May 23, 2025 08:53 — with GitHub Actions Inactive
@PatKamin PatKamin requested a review from pbalcer May 23, 2025 08:56
@PatKamin PatKamin marked this pull request as draft May 23, 2025 09:00
@PatKamin PatKamin marked this pull request as ready for review May 23, 2025 09:01
@PatKamin PatKamin temporarily deployed to WindowsCILock May 23, 2025 09:15 — with GitHub Actions Inactive
@PatKamin PatKamin temporarily deployed to WindowsCILock May 23, 2025 09:15 — with GitHub Actions Inactive
@pbalcer
Copy link
Contributor

pbalcer commented May 26, 2025

can you show a screenshot with how the new display names look like (on all types of charts)?

@PatKamin
Copy link
Contributor Author

can you show a screenshot with how the new display names look like (on all types of charts)?

Here are few examples, one for each type of charts (grouped results have updated legends, Historical Results charts - titles, for grouped results titles there will be another PR):
image
image
image

@pbalcer
Copy link
Contributor

pbalcer commented May 26, 2025

awesome, thanks!

@pbalcer
Copy link
Contributor

pbalcer commented May 26, 2025

@intel/llvm-gatekeepers please merge. The CI failure is unrelated.

@kbenzie kbenzie merged commit 4b7d11a into intel:sycl May 26, 2025
24 of 25 checks passed
@PatKamin PatKamin deleted the add-display-name branch May 26, 2025 14:28
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.

3 participants