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

Improve cascade plots based on feedback at P3HPC21 #9

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Pennycook
Copy link
Contributor

This commit makes several changes to the cascade plots:

  • The "cliff" connecting the last platform to the x-axis is made
    optional (and disabled by default). This declutters the plot
    and makes it simpler to explain.

  • Each platform is assigned a single character identifier that is
    rendered on top of the legend and platform map. This improves
    accessibility and makes choosing good colors less critical.

  • Minor assorted formatting changes to improve readability of axis and
    platform titles.

Co-authored-by: Jason Sewall [email protected]

This commit makes several changes to the cascade plots:

- The "cliff" connecting the last platform to the x-axis is made
  optional (and disabled by default). This declutters the plot
  and makes it simpler to explain.

- Each platform is assigned a single character identifier that is
  rendered on top of the legend and platform map. This improves
  accessibility and makes choosing good colors less critical.

- Minor assorted formatting changes to improve readability of axis and
  platform titles.

Co-authored-by: Jason Sewall <[email protected]>
@Pennycook
Copy link
Contributor Author

An example of an improved plot:
cascade-improvements

While reviewing these changes I remembered that there's a lot of magic in the Jupyter notebooks, and they don't diff very nicely. It would probably improve the user experience and make it easier for us to keep track of changes if we could move some of the new code (e.g. all the LegendHelper stuff) back into Python scripts somehow.

@JimCownie
Copy link

Looks much nicer, but wouldn't it be better to swap the placement of the two legends, so that the one for the lines is near the lines, and the one for the boxes is below the boxes?

@JimCownie
Copy link

p.s. there was an article on using Git with Jupyter notebooks recently https://www.fast.ai/2022/08/25/jupyter-git/ which may be useful.

@jasonsewall
Copy link

Strangely, I ran across that very article independently yesterday. Thanks for getting this out there, @Pennycook. I think @JimCownie's suggestion about switching the axes of those two legends is a fine one. Some magic might be needed to make very big platform lists fit.

@JimCownie
Copy link

FWIW, I also have some changes outside this function to ensure that if you are creating multiple plots of different (but intersecting sets of data) the encoding choices are common. So, say you have a number of plots comparing some set of compilers, each on a different machine, you want icx always to be orange, and LLVM always blue, etc. even if icx isn't available for all of the platforms (because they're Arm machines).
These tweaks are in https://github.com/UoB-HPC/p3hpc_sc22/blob/main/pp_vis.py but you may not have access :-(

@@ -152,7 +199,15 @@
}
],
"source": [
"fig = plt.figure(figsize=(12, 5))\n",
"fig = plt.figure(figsize=(16, 6.5))\n",
"app_order = [\"OpenMP\", \"Kokkos\", \"CUDA\", \"OpenACC\", \"OpenCL\"]\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: This is the approach to calling the script to ensure stable ordering of the applications so the application colours stay the same. The script is relatively unmodified, but invoked through the notebook with a list of applications and their orderings. By default, the script will order the applications with the most supported platforms first.

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.

4 participants