PR: Add documentation for Line Profiler plugin

vinisalazar commented 3 years ago

Hi,

this PR is part of the SciPy 2021 sprint.

Pull Request Checklist

[x] Read and followed this repo's Contributing Guidelines
[x] Based your PR on the latest version of the correct branch (master or 4.x)
[x] Checked your writing carefully for correct English spelling, grammar, etc
[x] Described your changes and the motivation for them below
[x] Noted what issue(s) this pull request resolves, creating one if needed

Description of Changes

Add page for lineprofile plugin
Add directory for the lineprofiler plugin images
Add images showcasing the use of the plugin

Issue(s) Resolved

Fix #273

juanis2112 commented 3 years ago

@jitseniesen we have now docs for our Line Profiler thanks to @vinisalazar who worked on it during our Scipy sprints! Please let us know what you think about this PR and if there is something we are missing.

vinisalazar commented 3 years ago

Once again, thank you for the review. I've applied the requested changes.

vinisalazar commented 3 years ago

Hi @jitseniesen,

thank you for the review. I've applied the requested changes, and added you as a co-author to the commits. I hope that's okay. If not, let me know and I can amend the commit messages.

Best, Vini

vinisalazar commented 3 years ago

@jitseniesen once again, thank you. I hope the PR is good to go now.

Best, Vini

jitseniesen commented 3 years ago

@juanis2112 I approved the PR. I think the next step is to press the button "Approve and run" under "1 workflow awaiting approval" but I'm hesitant to do so because I don't know what it means. Please explain and/or press the button yourself.

juanis2112 commented 2 years ago

@ccordoba12 I think you can merge this.

ccordoba12 commented 2 years ago

@CAM-Gerlach, could we merge this one as it is? Since it's already approved by @jitseniesen and @juanis2112, I think it's good to go.

Then you could open a new PR to make the changes you suggested and @juanis2112 could redo the gifs in another one.

CAM-Gerlach commented 2 years ago

@ccordoba12 I'm sorry for my extended absence causing these issues. Unfortunately, unlike PR #282 where I suggested exactly that, that's not really possible to do here, since other than one small comment and a few trivial technical/style changes that can simply be batch-applied in one commit and don't make any non-trivial changes to the content (and it appears another commit is needed anyway to trigger the CIs to run) my suggestions are all related to the images and GIFs being four times the nominal resolution and unoptimized, and the latter furthermore having crippling artifacts on non-hiDPI screens (they look bad enough in the screenshot but much worse in motion). If this PR is merged, that's a substantial chunk of wasted repo space and clone/sync bandwidth we will never be able to get back, because it will be impossible to delete the blobs from everyone's repos without re-writing history on master.

It would be better if its possible for @vinisalazar to re-generate the GIFs from the source video without the dithering, but I believe I should be able to more or less fix the problems using the GIFs as submitted. Thus, if @vinisalazar prefers, given the PR is otherwise ready to go, it would be straightforward for me to fix/optimize and rebase the existing images/GIFs on their branch for them, like I've done for every previous docs PR of this nature, and then it would be ready to merge with nothing further needed from anyone else.

vinisalazar commented 2 years ago

@CAM-Gerlach thank you for your review. I committed the suggestions that you've made. Unfortunately, I am currently unable to put any more time into this PR. Apologies for that. Please feel free to make the fixes or to reject the PR altogether.

Best, Vini

CAM-Gerlach commented 2 years ago

Thanks so much for your great work on this, @vinisalazar . Other than the scaling/optimization issues with the images/GIFs which I can fix for you, everything looks great. I'll go ahead and do that now, and then this should be ready to merge. Cheers!

CAM-Gerlach commented 2 years ago

Oh and by the way, great job on your detailed and well structured commit messages. I'll need to do a bit of light squashing to consolidate some of the smaller review commits and reduce the chance of merge conflicts when rebasing in the GIF/image changes, but I'll make sure to preserve your thoughtful commit structure and messages when I do so. Thanks!

vinisalazar commented 2 years ago

@CAM-Gerlach thanks a lot for your work in this PR :)

CAM-Gerlach commented 2 years ago

You're most welcome, but I really should be thanking you, as you did all the real work, along with @juanis2112 , @jitseniesen and @ccordoba12 .

ccordoba12 commented 2 years ago

Thanks @vinisalazar for your help! I'm really glad your two contributions to our docs are finally merged!

spyder-ide / spyder-docs