Skip to content

Add 'window' support to mprof to plot a particular time range #105

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

Merged
merged 2 commits into from
Nov 11, 2015
Merged

Conversation

pbowyer
Copy link
Contributor

@pbowyer pbowyer commented Nov 2, 2015

This is my contribution to address #103. It is my first time contributing to a Python project, so code review & feedback will be much appreciated.

My code lets one plot a particular time range out of the entire graph. This is really useful when bracketing functions in a long running, as it allows you to zoom in and see in detail which functions are being called.

To use, I added a new option called --window or -w, taking a comma-separated value of two numbers:

$ mprof plot --window 10,17.5 -o plot.png

One comment

  • I know my approach of passing options from plot_action to plot_file and thence to add_brackets is not a great. I couldn't think of a better way, short of putting the functions inside a class so a class variable can be accessed directly.

@fabianp
Copy link
Collaborator

fabianp commented Nov 3, 2015

Hi! Thanks a lot for this!

The code looks fine. It would be nice to have the possibility to only specify a start time or an end time, i.e. I would like to have

./mprof plot --window 0.1

that plots in the window 0.1 to end and

./mprof plot --window ,1.

that plots in the window from start to 1. . Do you think this is possible?

@pbowyer
Copy link
Contributor Author

pbowyer commented Nov 3, 2015

Heh, I wanted to add that too, but I couldn't work out how to get the max time in the code to automatically set the final condition 😄 If you can point me to that, I'll add it.

I'm not a fan of the syntax you propose as the first is open to interpretation (is it from 0 to the time, or from the time to the end?). I was thinking of the following, inspired by Python/numpy's list syntax:

$ mprof plot --window 0.1,:
$ mprof plot --window :,1

This way, it's explicit and can catch errors.

newvalue = [float(x) for x in value.split(',')]
except:
raise OptionValueError("'%s' option must contain two numbers separated with a comma" % value)
if len(newvalue) != 2:
Copy link
Collaborator

Choose a reason for hiding this comment

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

on my system I get an error because it doesn't find OptionValueError:
NameError: name 'OptionValueError' is not defined

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm using Python 3.4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the report, didn't realise Python wouldn't pick up missing imports until the line of code was run. Fixed with a modification to Line 12.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great. Now it just lacks support for open-ended windows (i.e. .1,:). Thanks!

@fabianp
Copy link
Collaborator

fabianp commented Nov 3, 2015

I don't think you need to know the end time in advance. You can do

pl.xlim((options.xlim[0], None))

for a limit on the start and

pl.xlim((None, options.xlim[1]))

for a limit on the end. So you just need to convert options.xlim[0] and options.xlim[1] to None if the limit is not present. I am OK with your syntax as long as it is documented.

@pbowyer
Copy link
Contributor Author

pbowyer commented Nov 5, 2015

Adding open-ended window support is taking a bit longer to do, as while None works, it then doesn't work for limiting which brackets are are added to the plot (which matters, as once added they show in the legend, regardless of whether in the viewport or not).

I have something working locally, but not happy with the code. You can view it at https://github.com/pbowyer/memory_profiler/compare/xlim...pbowyer:xlim--openwindow

@pbowyer
Copy link
Contributor Author

pbowyer commented Nov 6, 2015

I think it would be cleaner in our case to not use xlim and instead change the data being plotted to fit within our window, in https://github.com/fabianp/memory_profiler/blob/master/mprof#L361-L369

The first step is finding the entries in the time array that fit within our window, and then extracting the same slice from the other arrays.

This has extra benefits over xlim, like the y-axis rescaling automatically.

I've tried and failed as my array knowledge is not yet there for Python/numpy, so I'm leaving open-ended windows for someone else to implement. This PR for windows works well and I've a few people using it.

@fabianp
Copy link
Collaborator

fabianp commented Nov 11, 2015

Great, thanks for your effort. I'm merging it as-is then, we can take care of the rest later.

fabianp added a commit that referenced this pull request Nov 11, 2015
Add 'window' support to mprof to plot a particular time range
@fabianp fabianp merged commit d3c5742 into pythonprofilers:master Nov 11, 2015
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.

2 participants