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

Break up main into separate files and added CPU timeline #15

Open
wants to merge 43 commits into
base: paper-2.2
Choose a base branch
from

Conversation

lauren45983
Copy link

No description provided.

@lauren45983 lauren45983 added the wip Work in progress label Nov 4, 2020
Copy link
Member

@therishidesai therishidesai left a comment

Choose a reason for hiding this comment

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

Great work so far, just left a couple of comments to make the code a bit cleaner.

Comment on lines 7 to 11
populate_fps(trials, replaced_names)
populate_cpu(trials, replaced_names)
populate_gpu(trials, replaced_names)
populate_power(trials, replaced_names)
populate_mtp(trials, replaced_names)
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be a lot of similar code in these populate_x functions. Try to see if there is some way of creating a generic populate function so we don't have to have so much repeating code.

Comment on lines +13 to +14
stacked_cpu_time(data)
stacked_gpu_time(data)
Copy link
Member

Choose a reason for hiding this comment

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

Another area where there is repetitive code. Try to make a generic stacked graph function that you can pass either gpu, cpu, or energy data.

@mhuzai
Copy link
Member

mhuzai commented Dec 4, 2020

This is great work but I'm wondering if we should set a checkpoint and merge with master soon. Otherwise, reviewing/maintenance will keep getting progressively difficult.
Is this PR currently in a consistent/stable state such that it can reviewed and merged, @lauren45983 and @charmoniumQ?

@charmoniumQ
Copy link
Member

Everything works except Jeffrey's graphs, which I think work, but I can't test because I don't have the right data.

I propose @JeffreyZh4ng helps me get the data, I hotfix the new graphs, and then merge with master.

@lauren45983
Copy link
Author

lauren45983 commented Dec 4, 2020

From here, we plan to fix areas where things are repetitive and speed up the CPU timeline graph.

@lauren45983 lauren45983 closed this Dec 4, 2020
@lauren45983 lauren45983 reopened this Dec 4, 2020
@JeffreyZh4ng
Copy link

Everything works except Jeffrey's graphs, which I think work, but I can't test because I don't have the right data.

I propose @JeffreyZh4ng helps me get the data, I hotfix the new graphs, and then merge with master.

@charmoniumQ @lauren45983 someone should merge in isca-huzaifa-results branch to this one because that's where I did most of the work on the graphs. I believe it also has data

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants