-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: add plotting to benchmark suites #94
Conversation
if args.plot: | ||
files = list(os.scandir(bm.result_dir)) | ||
for file in reversed(files): | ||
if file.name[26:-4].replace('35', '3.5') == name: |
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.
What is 26:-4?
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.
to extract the suite name from the result file (remove date and .csv).
@@ -35,7 +46,7 @@ def run(args): | |||
print(f"Benchmark: Suite done in: {time.time() - start:.2f}s.") | |||
|
|||
df = report_to_dataframe(reports) | |||
save(df, name) | |||
file_name = save(df, name) |
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 logic here could be simplified by introducing a load call in the if args.plot?
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 issue with that is we still need to pass the .csv file name into plot()
so it can save the plot with the same name as the result. We can't generate the file name like here because in the if args.plot
part, we are using an old result so the timestamp will be wrong if we just pass in the DataFrame.
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.
Nice work - just wonder if the logic can be simplified a bit.
4fe75f9
to
59bce00
Compare
Improved version in PR #95. |
What this PR does / why we need it:
--plot
flag to plot from the last benchmark suite run results.Which issue(s) this PR addresses (if any):
Addresses #
Special notes for your reviewer:
Does this PR introduce a user-facing change?