nipy / PySurfer

Cortical neuroimaging visualization in Python
https://pysurfer.github.io/
BSD 3-Clause "New" or "Revised" License
240 stars 97 forks source link

save_image broken #209

Closed nfoti closed 6 years ago

nfoti commented 6 years ago

The save_image method tries to use the imsave function in scipy.misc. However, it appears that this function was removed from scipy some time ago (around 0.13). The imsave function now resides in matplotlib.pyplot.

You can recreate this bug with the following:

conda create -n mayavi python=3
source activate mayavi
conda install -c conda-forge mayavi scipy matplotlib
conda install ipython pyqt=4.11
pip install pysurfer

followed by:

ipython
$ %gui qt
$ from surfer import Brain
$ brain = Brain('fsaverage', 'split', ' 'inflated')
$ brain.save_image("./test.pdf")

(there's not really a need to do this in `ipython)

larsoner commented 6 years ago

However, it appears that this function was removed from scipy some time ago (around 0.13).

It was removed only 11 days ago. The reason it fails in your env is that they (up until 11 days ago master) require PIL/Imaging/pillow to be installed.

PySurfer should just directly use the functions there instead (which was always happening implicitly anyway), rather than using matplotlib.

larsoner commented 6 years ago

(correction: 11 days ago they were deprecated, they haven't actually been removed yet)

nfoti commented 6 years ago

Ah, I see. I missed the PIL dependence in the docs because it wasn't in the main list of dependencies. It might be worth it to do one of the following:

  1. add PIL/pillow to the dependencies list (in the docs) so it's more prominent
  2. add some sort of check to Brain.save_image() that checks for PIL/pillow and gives a more informative error than scipy.misc has no attribute "imsave"
  3. add this recipe to the "getting started" docs (and change the bit that says "PySurfer requires Python 2.7, and it does not work on Python 3"):
    $ conda create -n py3surfer python=3
    $ source activate py3surfer
    $ conda install -c conda-forge mayavi pillow scipy matplotlib
    $ pip install pysurfer
    $ python
    >>> from surfer import Brain
    >>> brain = Brain('fsaverage', 'split', 'inflated')
    >>> brain.save_image('~/Desktop/test-pysurfer-imsave.pdf')

    ...and note that for it to work in python 3 requires pillow (no py3 version of PIL).

larsoner commented 6 years ago

Let's add pillow to the list of requirements, and use it directly for saving images instead of scipy. Do you have time to make a PR?

christianbrodbeck commented 6 years ago

Does matplotlib depend on PIL? https://github.com/matplotlib/matplotlib/blob/5f63dd42ec6deec5c85ab975d7d879c7c81cb161/lib/matplotlib/image.py#L1305

larsoner commented 6 years ago

I don't know, but I'd rather have a dependency on pillow than matplotlib, it's lighter and Mayavi often has conflicts with matplotlib

mwaskom commented 6 years ago

Don't we already have a dependency on matplotlib?

christianbrodbeck commented 6 years ago

Yeah I also thought that we already depend on Matplotlib for colormaps, for example

christianbrodbeck commented 6 years ago

Looks like at the moment matplotlib is a lazy import, but it's used by a function called by Brain.init (https://github.com/nipy/PySurfer/blob/master/surfer/viz.py#L561)

larsoner commented 6 years ago

Ahh I forgot about that, good call. Yeah let's do matplotlib then.

It looks like PIL / pillow is only needed for some formats:

Matplotlib depends on Pillow for reading and saving JPEG, BMP, and TIFF image files.

mwaskom commented 6 years ago

I think instead of using matplotlib's imsave function we should add two of our own:

The advantage of this implementation is the first method alone will be useful in the jupyter notebook as you could call, e.g. b.make_image at the end of a cell and it will show your screenshot inline. I define little helper functions in notebooks all the time to do this, and it would be nice if it was a Brain method.

Alternative implementations include b.screenshot(as_figure=True), though I think I like that less.

larsoner commented 6 years ago

Going the figure->savefig route requires matplotlib backend imports, GUI usage, etc. It seems a bit overkill for this use case (and can create conflicts). So I'd rather avoid that route for the task of just saving an image, assuming matplotlib does expose something that takes a data array and writes it to disk as PNG. Then the figure-generation stuff can be taken care of separately.

mwaskom commented 6 years ago

The function @christianbrodbeck creates a figure (though there is a route to write pngs that avoids doing so).

larsoner commented 6 years ago

Looking at the matplotlib code, it uses Agg directly so I don't think there should be problems there actually. I'll make a PR