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

PR: Show time units in Profiler #3741

Merged
merged 16 commits into from
Dec 18, 2016
Merged

Conversation

malliwi88
Copy link
Contributor

@malliwi88 malliwi88 commented Nov 24, 2016

Fixes #3672.


Maybe it is necessary to do a little cleaning to the profiler, I found a lot of code not functional inside profiler.

The final result looks like this
spyder 3 1 0 dev0 python 3 5 _001

@malliwi88
Copy link
Contributor Author

malliwi88 commented Nov 24, 2016

@ccordoba12

I waiting for test for check if conflicts (on #3735) are solved now

@malliwi88 malliwi88 changed the title Units on profiler feature/ Show time units on profiler Nov 24, 2016
@malliwi88 malliwi88 changed the title feature/ Show time units on profiler PR: Show time units on profiler Nov 24, 2016
@goanpeca
Copy link
Member

@ccordoba12 is this ready?

@ccordoba12 ccordoba12 added this to the v3.1 milestone Nov 27, 2016
@@ -510,25 +510,70 @@ def color_string(self, args):
color = "red"
return [format[0] % x[0], [diff_str, color]]

def format_measure(self, data):

Copy link
Member

Choose a reason for hiding this comment

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

Please add a docstring here

formated_data = [0] * len(data)

for (i, measure) in enumerate(data):

Copy link
Member

Choose a reason for hiding this comment

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

Remove this blank space

data = [x.stats.get(child_key, [0,0,0,0,0]) for x in self.stats1]
# print(self.stats[child_key], list(zip(*data))[1:4])
#return map(self.color_string,
# zip(list(zip(*data))[1:4], [["%i n"] * 2, ["%.3f ms", "%.3f ms"], ["%.3f", "%.3f"]]))
Copy link
Member

Choose a reason for hiding this comment

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

Remove these commented lines if they are not necessary anymore

measures = data[0][4][k]

calls = list(measures[:1]) # measures[:2]

Copy link
Member

Choose a reason for hiding this comment

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

Remove this blank line

cum_time_dif)) = self.format_output(child_key)
(primcalls, total_calls, loc_time, cum_time, callers
) = self.stats[child_key]
"""
Copy link
Member

Choose a reason for hiding this comment

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

Remove this block of commented code if it's not necessary anymore.

child_item.setData(6, Qt.DisplayRole, total_calls_dif[0])
child_item.setForeground(6, QColor(total_calls_dif[1]))
child_item.setTextAlignment(6, Qt.AlignLeft)
"""
Copy link
Member

Choose a reason for hiding this comment

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

Again, remove this if it's not necessary anymore.

@ccordoba12
Copy link
Member

ccordoba12 commented Nov 27, 2016

No, it's not. There's a lot of commented (and it's seems unnecessary) code that @malliwi88 needs to decide if he wants to leave or remove.

@malliwi88
Copy link
Contributor Author

Ok @ccordoba12 your suggestions have been added and the test has passed :+1,

formated_data = [0] * len(data)

for (i, measure) in enumerate(data):

Copy link
Member

Choose a reason for hiding this comment

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

Remove this blank line

@malliwi88
Copy link
Contributor Author

@ccordoba12 #3672 and #3735, they are solved

Now profiler is showing time units. I think that maybe it is necessary to do a little cleaning to the profiler, I found a lot of code not functional inside profiler.

At the beginning the profiler looks like this:
profiler_before

The final result looks like this:
spyder 3 1 0 dev0 python 3 5 _001

Any suggestions or corrections are welcome.

@goanpeca
Copy link
Member

@malliwi88 just focus on the problem at hand. The PR should solve the immediate and specific problem not refactor a bunch of code.


child_item.setData(2, Qt.DisplayRole, cum_time_dif[0])
child_item.setForeground(2, QColor(cum_time_dif[1]))
child_item.setTextAlignment(2, Qt.AlignLeft)
Copy link
Member

Choose a reason for hiding this comment

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

What was this doing and why did you remove it?


child_item.setData(4, Qt.DisplayRole, loc_time_dif[0])
child_item.setForeground(4, QColor(loc_time_dif[1]))
child_item.setTextAlignment(4, Qt.AlignLeft)
Copy link
Member

Choose a reason for hiding this comment

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

Also this?


child_item.setData(6, Qt.DisplayRole, total_calls_dif[0])
child_item.setForeground(6, QColor(total_calls_dif[1]))
child_item.setTextAlignment(6, Qt.AlignLeft)
Copy link
Member

Choose a reason for hiding this comment

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

And this?

@malliwi88
Copy link
Contributor Author

malliwi88 commented Dec 1, 2016

@ccordoba12 ready 👍, You could check this.

@ccordoba12 ccordoba12 changed the title PR: Show time units on profiler PR: Show time units in Profiler Dec 18, 2016
@ccordoba12 ccordoba12 merged commit 7756b78 into spyder-ide:3.x Dec 18, 2016
ccordoba12 added a commit that referenced this pull request Dec 18, 2016
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