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

Analysis: added matplotlib figure generation with several desirable performance metrics. #612

Open
wants to merge 31 commits into
base: development
Choose a base branch
from

Conversation

nikwl
Copy link

@nikwl nikwl commented May 21, 2021

Thanks so much for this awesome tool!

I've been wanting to see a more graphical representation of how my plots are doing. This pr adds the option to create a matplotlib figure that summarizes your plotting performance, including:

  • Dumbbell plot visualizing start, end, and duration of your plots over time.
  • Average plot rate over time (computed over a sliding window).
  • Average plot time (computed over a sliding window).
  • Cumulative number of plots over time.

plotman_figure

It adds two new arguments to plotman analyze:
--logdir: a directory that contains a number of log files (this can be used instead --logfile).
--figfile: path to save the figure to.

It adds one new dependency: matplotlib

The figure creation is done by high level functions which are easily removable or interchangeable. If the maintainers don't like my method of integration, figure creation should be easy to reintegrate in a better way. Or if using matplotlib is undesirable then hopefully the body of the functions can be reused in some way to still be helpful for doing analysis.

Thanks!

@nikwl nikwl marked this pull request as draft May 21, 2021 23:46
@rafaelsteil
Copy link
Contributor

Here's a little script I created that generated a PDF with charts using data from plotman export rafaelsteil@e4a0632

Copy link
Collaborator

@altendky altendky left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution here. I apologize for leaving it hanging for so long.

What do you think of separating it from analyze and making it its own plotman graph subcommand or such? I think it could get its own plotman.graph module as well.

setup.cfg Outdated
@@ -45,6 +45,7 @@ install_requires =
psutil ~= 5.8
pyyaml ~= 5.4
texttable ~= 1.6
matplotlib ~= 3.4.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should probably be an extra down below for plotting graphing.

graph =
    matplotlib ~= 3.4
    numpy ~= 1.20

Copy link
Collaborator

@altendky altendky left a comment

Choose a reason for hiding this comment

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

It looks like there may have been some confusion with the catch up merge. Take a look down through the diff and make sure all the changes are intended to be part of this PR. Sorry for my role in this what with taking forever to catch up on the community contributions.


# Read the logfile, triggering various behaviors on various
# regex matches.
for line in f:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need dedicated log parsing here? If the existing parsing doesn't catch everything, maybe we could add the extra bits there? Though I have to admit we have two parsers right now. I was hoping to make some time to get everything using the newer one. I'll try to get that done first.

src/plotman/plotman.py Outdated Show resolved Hide resolved
f.savefig(figfile)


if __name__ == '__main__':
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it particularly needed for this to be directly runnable rather than just using the plotman CLI access?

@nikwl
Copy link
Author

nikwl commented Jun 23, 2021

Hey @altendky, thanks for checking this out and for all of your feedback. I really only got a chance to bring the fork up to speed yesterday - I do mean to fix all the above issues but I'm a little pressed for time right now and I didn't get a chance to double check anything or actually test the code. When I do finally get everything working I'll "un-draft" the pr.

Thanks!

@altendky
Copy link
Collaborator

Awesome, thanks. Don't rush on my behalf.

@nikwl nikwl marked this pull request as ready for review August 7, 2021 20:08
@nikwl
Copy link
Author

nikwl commented Aug 7, 2021

Hey @altendky, sorry this has taken so long but I finally got around to updating this pr. If you could look it over when you get a chance that would be awesome. Thanks!

Copy link
Collaborator

@altendky altendky left a comment

Choose a reason for hiding this comment

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

I think we're close. Still sorry for my slowness... again.

src/plotman/job.py Outdated Show resolved Hide resolved
src/plotman/plotman.py Outdated Show resolved Hide resolved
@altendky
Copy link
Collaborator

Oh, and add a changelog entry.

nikwl added 3 commits August 28, 2021 20:26
…s merge. Current state should exactly reflect ericaltendorf/plotman except in plotman.py and graph.py. Also added a changelog entry
@nikwl nikwl requested a review from altendky August 29, 2021 00:48
@altendky
Copy link
Collaborator

Maybe try nikwl#1 to catch up and resolve the conflicts.

@nikwl
Copy link
Author

nikwl commented Aug 29, 2021

@altendky Thanks so much for the pr, that made things much, much simpler. I've updated the log parsing and tested it on my plots, seems to be working.

@altendky altendky dismissed their stale review August 30, 2021 12:44

requested changes made

@altendky
Copy link
Collaborator

Welp, a bit closer with the hints. If you aren't sure how to handle these I can take a look later.

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