nipy / PySurfer

Cortical neuroimaging visualization in Python

https://pysurfer.github.io/

BSD 3-Clause "New" or "Revised" License

240 stars 97 forks source link

MRG: Fix imsave #221

Closed larsoner closed 6 years ago

larsoner commented 6 years ago

Closes #209.

@mwaskom I used your approach but left the figure-generation private for now. Feel free to make a follow-up PR to make it public with proper documentation, etc.

mwaskom commented 6 years ago

Haven't tested but LGTM

codecov-io commented 6 years ago

Codecov Report

Merging #221 into master will increase coverage by 7.49%. The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
+ Coverage   66.81%   74.31%   +7.49%     
==========================================
  Files           8        8              
  Lines        2438     2441       +3     
  Branches      473      485      +12     
==========================================
+ Hits         1629     1814     +185     
+ Misses        624      454     -170     
+ Partials      185      173      -12

larsoner commented 6 years ago

Okay CIs are happy (except pending OSX but that usually takes forever). I added a 3.6 CI, fixed related errors, and fixed a couple of bare except:s we had. Ready for review/merge from my end.

larsoner commented 6 years ago

(actually not sure about AppVeyor yet, either, but it passed last time I let it finish)

christianbrodbeck commented 6 years ago

Is there an advantage over using Matplotlib's imsave (https://github.com/matplotlib/matplotlib/blob/5f63dd42ec6deec5c85ab975d7d879c7c81cb161/lib/matplotlib/image.py#L1305), which seems to be more efficient for PNG but do the same otherwise?

larsoner commented 6 years ago

I assumed it was just an efficiency thing so I didn't include it to reduce complexity, but I don't know for sure

christianbrodbeck commented 6 years ago

Oh, I didn't mean to include the special case, but whether it wouldn't be easier to just use matplotlib.image.imsave

larsoner commented 6 years ago

This moves toward a use case @mwaskom wanted, see his comments toward the end of #209

christianbrodbeck commented 6 years ago

Aah, makes sense

On Nov 27, 2017, at 9:21 PM, Eric Larson notifications@github.com wrote:

This moves toward a use case @mwaskom https://github.com/mwaskom wanted, see his comments toward the end of #209 https://github.com/nipy/PySurfer/issues/209 — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nipy/PySurfer/pull/221#issuecomment-347392650, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI5a5pmweM1CpgZiXtKpgzimeP7t1k6ks5s624igaJpZM4Qr8Mk.

larsoner commented 6 years ago

Anybody else want to look (or @mwaskom want to look again now that everything passes)? If not I'll merge at the end of the week