Image tutorial notebook edit #3402
Conversation
| @@ -209,7 +176,7 @@ object: | |||
|
|
|||
| .. sourcecode:: ipython | |||
|
|
|||
| In [8]: imgplot.set_cmap('hot') | |||
| In [8]: plt.imshow(lum_img, cmap="hot") | |||
There was a problem hiding this comment.
Please leave the set_cmap in at least one of the cases.
There was a problem hiding this comment.
I stripped them out because in the IPython notebook, you can't use them from one cell to the next. Within a cell, they're still just fine. I'll make a note to that extent. This was the issue that tripped up someone I was training on MPL usage - he was trying to use the notebook, and wondering why several commands from this tutorial didn't work.
|
I actually thing the use of |
msarahan
force-pushed
the
ImageTutorialNotebookEdit
branch
from
227db6d
to
9af60e6
Compare
|
Botched squash - please don't merge until I get this figured out (sorry) |
msarahan
force-pushed
the
ImageTutorialNotebookEdit
branch
2 times, most recently
from
b0bbeae
to
4daffe4
Compare
|
OK, I hope the squash cleans things up. I have added text about backends that I hope is perfectly clear. Please let me know if you have further ideas for improvement. |
| In [10]: rsize = img.resize((img.size[0]/10,img.size[1]/10)) # Use Pillow to resize | ||
| In [11]: rsizeArr = np.asarray(rsize) # Get array back | ||
| In [12]: imgplot = plt.imshow(rsizeArr) | ||
| In [15]: import Image |
There was a problem hiding this comment.
This should be from PIL import Image Support for importing from outside the PIL namespace was dropped in Pillow
|
The docs build fail due to the issue with image above |
msarahan
force-pushed
the
ImageTutorialNotebookEdit
branch
from
fcd64fa
to
13baadd
Compare
|
Fixed and re-squashed. Thanks for pointing that out. |
|
This looks good. The indentation changes to the plot directive blocks makes me a bit nervous, have you verified the docs still build right? |
|
You are rightfully nervous - some earlier commits in there had issues. I have checked it, and it all looks fine in HTML on my built docs. IPython code blocks format fine, and figures are all there. |
|
This looks good to me. One last thing. Could you replace the use of %pylab with %matplotlib and delete the remarks about dropping IPython is actively discouraging the use of various forms of |
msarahan
force-pushed
the
ImageTutorialNotebookEdit
branch
2 times, most recently
from
bfda6a2
to
91b49a2
Compare
|
Yes, that was an excellent suggestion. Much clearer, and definitely better to get away from pylab namespace pollution |
|
|
||
| $ipython | ||
| $ipython --matplotlib |
There was a problem hiding this comment.
Just stumbled across this, and a small question: is there a reason you use (and thus recommend) --matplotlib intead of the cell magic %matplotlib. I personally think using the flag is always a bad habit, as using %matplotlib makes sure someone else can run your notebook as is (and does not need to start the notebook kernel in a specific way)
There was a problem hiding this comment.
I don't think the recommendation is to use that flag at all. I didn't think
the ipython would have made it available after the lessons leared about the
--pylab (which, along with the namespace issues, brings the exact issues
you raise). Even if ipython makes it available, I don't think we should be
recommending it.
On Fri, Nov 21, 2014 at 9:09 AM, Joris Van den Bossche <
notifications@github.com> wrote:
In doc/users/image_tutorial.rst:
- $ipython
- $ipython --matplotlib
Just stumbled across this, and a small question: is there a reason you use
(and thus recommend) --matplotlib intead of the cell magic %matplotlib. I
personally think using the flag is always a bad habit, as using
%matplotlib makes sure someone else can run your notebook as is (and does
not need to start the notebook kernel in a specific way)—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/3402/files#r20716168.
There was a problem hiding this comment.
I would also vote for only mentioning the %matplotlib magic and not including the flag at all.
My understanding is that IPython will only allow the flag in the consoles and not the notebook from version 3.0 due to the reproducibility issue. The reproducibility is much less of an issue for the console. (the same will be true for for --pylab and --matplotlib)
There was a problem hiding this comment.
I thought the issue with the flags was that they are python specific and
the notebook sever needs to be able to deal with all kernels.
On Fri, Nov 21, 2014, 09:22 Jens Hedegaard Nielsen notifications@github.com
wrote:
In doc/users/image_tutorial.rst:
- $ipython
- $ipython --matplotlib
I would also vote for only mentioning the %matplotlib magic and not
including the flag at all.My understanding is that IPython will only allow the flag in the consoles
and not the notebook from version 3.0 due to the reproducibility issue. The
reproducibility is much less of an issue for the console. (the same will be
true for for --pylab and --matplotlib)—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/3402/files#r20716778.
There was a problem hiding this comment.
@tacaswell yes I think that is a second argument but the zmq text console and qt console could also be multi language and here it still works in the master build that I have. In any event this tutorial will be simpler if it uses %matplotlib which should work in all IPython variations.
- expand some text regarding backend usage and IPython notebook plot - improve style a bit (PEP 8) - shorten array printout - convert image resizing example to use PIL - replace pyplot discussion in top material - remove matplotlib argument to IPython in intro
msarahan
force-pushed
the
ImageTutorialNotebookEdit
branch
from
0766e86
to
e2616e8
Compare
|
Loud and clear - good to keep this current. Anything else? |
|
Looks good to me. Thanks for your effort and patience. This unfortunately needs a rebase now. I will merge it via #3830 |
|
@jenshnielsen any idea why it needed a rebase? Just for my own education. I tried to rebase using git rebase -i HEAD~7 7 corresponding to the number of commits I counted in the log that I wanted to squash into one. I ended up with about 15 commits in the resulting squash - several earlier commits from Matplotlib. This was the cause of the "botched squash" - when I saw this, I just exited the rebase by closing the text editor. Git then proceeded with processing the rebase. After this, the next time I did the rebase command, things behaved normally - only the desired 7 commits showed up, and I squashed them into one. When I forced this update to Github, Github pulled in 7 earlier commits into this PR. I "fixed" this by doing yet another git rebase, and removing those 7 earlier commits, effectively positioning my one squashed commit earlier in the tree. I guess it worked, but I'd really like to understand better so I can do this more cleanly next time, and ideally save rebasing work for other people like you. Please don't feel obliged to explain it here - pointing me to links for me to read would be just as helpful. |
|
It is actually mostly my fault. Your branch only contained one new commit due to your rebase as it should be. It turns out that between when you branched from master and now another commit was merged that removed the reference to pylab from the tutorial. This means that your branch conflicted with this commit because you both modified the same line. To get around this with a rebase you would have to do where origin is the remote name that you use for the main matplotlib repo. When you do that it gives a conflict since both modified the same line. You will then have to modify the file manually to resole the conflict. In summary rebase can be used for two similar but different things. Rewriting history on a branch as you did and rebasing your commits on top of another branch (i.e. a new version of master) which is needed to resolve the conflicts. I actually resolved the conflict with a manual merge into master and manually chose your version of the introduction over the other versions (also without pylab) because yours seemed better. |
|
Hope the above is useful otherwise please ask again. |
|
That is helpful - thanks! |
Make the Image tutorial more friendly to people using the IPython notebook to follow along (avoid use of set_xxx functions, use arguments to imshow instead.)
Also: