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

[wip] gallery example for developers gallery: execution times of scikit-image functions #5214

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

Conversation

emmanuelle
Copy link
Member

This is work in progress, but I'm still interested in a quick round of high-level comments about the example and the visualization.

@pep8speaks
Copy link

Hello @emmanuelle! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 55:80: E501 line too long (82 > 79 characters)
Line 82:80: E501 line too long (87 > 79 characters)
Line 199:80: E501 line too long (81 > 79 characters)
Line 212:13: E722 do not use bare 'except'
Line 220:1: E303 too many blank lines (3)
Line 270:80: E501 line too long (80 > 79 characters)
Line 276:80: E501 line too long (107 > 79 characters)
Line 278:13: E126 continuation line over-indented for hanging indent
Line 287:47: E231 missing whitespace after ':'
Line 288:9: E126 continuation line over-indented for hanging indent

@grlee77
Copy link
Contributor

grlee77 commented Feb 2, 2021

I just started looking at this (I have run the script and looked at the output, but haven't reviewed the implementation) and it is pretty nice.

I like that it sorts by both module and execution time. I also like that having visualization in the browser enables hovering over specific data points to get the corresponding function name (and line profiler details). Oddly, the popup doesn't appear for me on a handful of isolated points, though

It would also be nice to add an option to save the DataFrame to disk (or maybe export only the function names / times in a table as CSV?).

It might also be interesting to check relative performance across image sizes as well. (200, 200) keeps the overall runtime reasonably short, but we can run offline for (1024, 1024) or something like that where the image is larger than the typical CPU L2/L3 cache size to see if the same relative ordering holds.

@grlee77
Copy link
Contributor

grlee77 commented Feb 2, 2021

I see inpaint_biharmonic was disabled due to long execution times. I was recently looking at this function and made some performance improvements. I should have a PR in for it soon.

can probably omit skimage.filters.try_all_thresholds too since the functions it calls are already benchmarked

@emmanuelle
Copy link
Member Author

Thanks for taking a look Greg, this is exactly the kind of early feedback I was looking for :-). As for the popup I need to investigate, probably it's when the hover information is too long.

I used a 200x200 image so that the total execution time stays reasonable, but yes, the text of the example should suggest to try it for other sizes, and that the ranking of functions might change.

Base automatically changed from master to main February 18, 2021 18:23
@soupault soupault marked this pull request as draft October 26, 2023 13:52
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.

None yet

3 participants