-
Notifications
You must be signed in to change notification settings - Fork 2
Added process gpu timeline and combining different reports #26
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
| @@ -0,0 +1,62 @@ | |||
| #!/usr/bin/env python3 | |||
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.
How is combine_reports related to process_gpu_timeline? Should they be separate PR's?
| from pathlib import Path | ||
|
|
||
|
|
||
| def geometric_mean(values): |
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.
This function already exists here
| return np.exp(np.mean(np.log(values))) | ||
|
|
||
|
|
||
| def process_gpu_timeline(reports_dir, use_geo_mean=False): |
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.
This looks very similar in functionality to
| def process_gpu_timeline_data(sweep_dir, use_geo_mean=False): |
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.
Hi Ian, these two functions are different, other than reading an excel and grouping by type, nothing is almost common. Referred one read a different directory structure and produce an excel with thread and channel information, whereas the one in the PR is a lot simplified version, it reads a directory structure without thread and channel information and hence produce a lot simpler excel. We can refactor them to use same base, but the common part is just reading excel and group by type along with the mean calculation (mean/geometric mean). Rest all will be different.
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.
A sample implementation of what I explained : #33
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.
Is there a reason we need to have a different directory structure for gemm_analysis? Seem to me that we could better abstract and combine the two single config and gemm analysis to use the same overarching function and pass in the types that you want to group along as an arg and the gemm analysis can loop through the larger set of configs with a maybe slightly modified file structure that it sweeps through.
For sake of time this could be done later if you add a TODO comment of how we could better merge these two
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.
Essentially GEMM is done for sweep run which is 8 single config (2 thread x 4 channels as of now).
Tracelens dumps comes in different format .. But we can create an abstraction to read the excel file in single config and iteration in sweep analysis (GEMM). Something what we already did in #33 but with some more abstraction.
Orignal PR : #23 : Split across mutiple prs
subpr : 2
Files :