Skip to content

Conversation

@drammock
Copy link
Member

@drammock drammock commented Aug 16, 2019

closes #6647

  • raw
  • epochs
  • ICA

@drammock
Copy link
Member Author

@codecov
Copy link

codecov bot commented Aug 16, 2019

Codecov Report

Merging #6670 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6670      +/-   ##
==========================================
+ Coverage   89.39%   89.48%   +0.09%     
==========================================
  Files         415      420       +5     
  Lines       75230    75589     +359     
  Branches    12366    12383      +17     
==========================================
+ Hits        67249    67638     +389     
+ Misses       5149     5142       -7     
+ Partials     2832     2809      -23

@drammock drammock changed the title WIP: make subplots_adjust work with {raw,epochs,ica}.plot() ENH: make subplots_adjust work with {raw,epochs}.plot() and ica.plot_sources() Aug 16, 2019
@lgtm-com
Copy link

lgtm-com bot commented Aug 17, 2019

This pull request introduces 5 alerts when merging 3a3666f into 0ad37f0 - view on LGTM.com

new alerts:

  • 3 for Unhashable object hashed
  • 2 for Unused import

@cbrnr
Copy link
Contributor

cbrnr commented Aug 17, 2019

Any chance you can also implement a zen mode (i.e. get rid of the buttons and the timeline overview)?

@drammock
Copy link
Member Author

drammock commented Aug 17, 2019 via email

@drammock drammock changed the title ENH: make subplots_adjust work with {raw,epochs}.plot() and ica.plot_sources() MRG, ENH: make subplots_adjust work with {raw,epochs}.plot() and ica.plot_sources() Aug 18, 2019
@drammock
Copy link
Member Author

drammock commented Aug 18, 2019

CIs are happy except for Circle (complaining about dipy link, see #6671). Ready for review and merge. Someone with a hi-dpi screen (@larsoner?) should try generating and resizing the raw.plot() window, then calling subplots_adjust(), to make sure everything looks normal.

@larsoner
Copy link
Member

@cbrnr also feel free to try. I'll give it a shot tomorrow

@cbrnr
Copy link
Contributor

cbrnr commented Aug 19, 2019

I tried raw.plot on my hidpi screen and it works perfectly! +1 for merge (maybe @larsoner can you test this for epochs and/or ICA?).

@larsoner
Copy link
Member

On my HiDPI macOS screen things do not look correct. The following code:

import mne
path = mne.datasets.sample.data_path()
raw = mne.io.read_raw_fif(path + '/MEG/sample/sample_audvis_raw.fif')
raw.plot()
events = mne.find_events(raw)
epochs = mne.Epochs(raw, events)
epochs.plot()

Shows this (just the raw plot here but epochs has the same spacing issues) on master, which has no overlap between elements (correct):

Screen Shot 2019-08-19 at 10 37 16

But on this PR it looks like:

Screen Shot 2019-08-19 at 10 37 34

I am not sure why I have these overlap problems but @cbrnr did not (unless you missed them?).

@larsoner
Copy link
Member

If you want to test on a non-HiDPI screen as long as you're on the Qt5Agg backend you can do (here on my Linux non-HiDPI machine):

QT_SCALE_FACTOR=2 python -ui ~/Desktop/plot.py

I get on master:

Screenshot from 2019-08-19 10-46-51

But on this PR:

Screenshot from 2019-08-19 10-47-05

@drammock
Copy link
Member Author

drammock commented Aug 19, 2019 via email

@drammock drammock force-pushed the raw_subplots_adjust branch from d336cdb to 9889cff Compare August 19, 2019 19:04
@drammock
Copy link
Member Author

@larsoner @cbrnr the changes just pushed work locally (show correct button alignment / no overlap) when testing with QT_SCALE_FACTOR=2 (or 3 or 4). Please double-check, but assuming it's good (and the CIs are happy) this should be ready for merge.

@drammock
Copy link
Member Author

@larsoner
Copy link
Member

The gaps at the top and bottom seem incorrect. master:

Screen Shot 2019-08-19 at 20 39 17

This PR:

Screen Shot 2019-08-19 at 20 39 34

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the deep dive into mpl @drammock. Works on my macOS HiDPI screen and your Linux tests look conclusive. Example also looks good:

https://14782-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/preprocessing/plot_15_handling_bad_channels.html

+1 for merge from me

@cbrnr
Copy link
Contributor

cbrnr commented Aug 21, 2019

Unfortunately now it doesn't work for me - but only if I'm on a scaled resolution and using the osx backend (qt5 works fine). After resizing the window (subplots_adjust works fine), the margins are wrong:

import mne
path = mne.datasets.sample.data_path()
raw = mne.io.read_raw_fif(path + '/MEG/sample/sample_audvis_raw.fif')
raw.plot()

Initially, this looks fine:
Screen Shot 2019-08-21 at 08 52 57

After manually resizing the window:
Screen Shot 2019-08-21 at 08 53 46

@larsoner
Copy link
Member

I also see this when running with MPLBACKEND=macosx. This is almost certainly a matplotlib bug in that backend (it tends to be buggy IIRC). That backend is also just about unusable for me it's so slow. I wonder if we should really support it.

@drammock is there something I could try adding as a resize event handler (for the macosx backend only) that might force it to update the params?

@drammock
Copy link
Member Author

I'm really not sure what to do here. I can't tell from the screenshots whether resizing is not happening at all, or if it's happening but not to the correct degree.

@cbrnr
Copy link
Contributor

cbrnr commented Aug 21, 2019

Let's not drop osx support. This is still the default backend and I don't find it slower than qt5 (what's really slowing my Mac down to a point where it's almost unusable is scaled resolution aka HiDPI, but that's another story).

@drammock
Copy link
Member Author

I have arranged access to a Mac for next week so I'll try to debug on Monday. I tried today on KC's Mac desktop but could not reproduce (his resolution may not be high enough? It is an "Apple Cinema display" 2560 x 1440)

@larsoner
Copy link
Member

The Qt5Agg backend is fine, did you do it with MPLBACKEND=MacOSX python -ui test.py or so?

@drammock
Copy link
Member Author

I tested only with the default backed but had to run to a meeting before I could verify whether it was defaulting to qt or macosx. Will continue testing tomorrow.

@cbrnr
Copy link
Contributor

cbrnr commented Aug 23, 2019

@drammock you need to set a scaled resolution, it works with the native resolution. I'm not sure this works with 2560x1440, but try setting it to a scaled (lower) resolution. I have a 4K display and select the second resolution to the right in the Displays settings page.

@drammock
Copy link
Member Author

I'm really struggling to reproduce this. Screenshot:

Screen Shot 2019-08-24 at 1 04 57 PM

@drammock
Copy link
Member Author

drammock commented Aug 24, 2019

running in a plain python terminal instead of ipython, the mouse cursor never turns to double-headed arrow so it looks like resizing doesn't work --- note also the grayed out close/minimize buttons even though the window is on top --- but if I click and drag the window edge anyway and then go back to the terminal and start typing, the window size does belatedly change (and behavior of the margins is as expected). Before resize:

Screen Shot 2019-08-24 at 1 16 16 PM

After subplots_adjust and resize:

Screen Shot 2019-08-24 at 1 21 30 PM

@drammock
Copy link
Member Author

Running as a script I also get expected behavior:

Screen Shot 2019-08-24 at 1 28 19 PM

@drammock
Copy link
Member Author

Here are the test system details:

(mnedev) D-10-19-149-242:~ labshared$ python -c 'import mne; mne.sys_info()'
Platform:      Darwin-17.4.0-x86_64-i386-64bit
Python:        3.7.4 (default, Aug 13 2019, 15:17:50)  [Clang 4.0.1 (tags/RELEASE_401/final)]
Executable:    /Users/labshared/miniconda3/envs/mnedev/bin/python
CPU:           i386: 12 cores
Memory:        16.0 GB

mne:           0.19.dev0
numpy:         1.16.4 {blas=mkl_rt, lapack=mkl_rt}
scipy:         1.3.1
matplotlib:    3.1.0 {backend=MacOSX}

sklearn:       0.21.2
numba:         0.45.1
nibabel:       2.5.0
cupy:          Not found
pandas:        0.25.0
dipy:          1.0.0
mayavi:        4.7.1 {qt_api=pyqt5, PyQt5=5.9.2}
pyvista:       0.22.1
vtk:           8.1.2

@drammock
Copy link
Member Author

Same results at 2048x1152 resolution:

Screen Shot 2019-08-24 at 1 39 43 PM

...and at 1280x720:

Screen Shot 2019-08-24 at 1 44 10 PM

@drammock
Copy link
Member Author

for completeness here are the Qt5Agg screenshots on the same system. Default scaling:

Screen Shot 2019-08-24 at 1 53 07 PM

With QT_SCALE_FACTOR=2:

Screen Shot 2019-08-24 at 1 56 20 PM

@cbrnr
Copy link
Contributor

cbrnr commented Aug 26, 2019

I think your monitor might not be recognized as a retina/HiDPI screen because its native resolution is too low. To reproduce the problem, you need to have a screen that macOS recognized as retina/HiDPI capable. Here's what my display preferences window looks like - notice the five presets for different scaling factors:

Screen Shot 2019-08-26 at 09 17 49

I think this is only shown if the monitor is at least 4K, and these settings seem to influence how scaling is performed. In your case, I think macOS performs regular non-HiDPI scaling, and therefore you don't see the problem. I can simulate this even on my HiDPI screen by alt-clicking on the "Scaled" radio button and selecting a resolution that says "(low resolution)":

Screen Shot 2019-08-26 at 09 19 37

Now this is exactly the same 3360 x 1890 resolution I normally use, but in low resolution mode (aka non-HiDPI) I don't see the issue with overlapping elements either.

@cbrnr
Copy link
Contributor

cbrnr commented Aug 26, 2019

PS: What this means is that this issue occurs on MacBooks with retina displays (so basically all of them) with the default osx backend (so no external display required). This is even worse, because there is no way to select the native (full) resolution for internal displays (because for whatever reason, even the maximum resolution is a scaled one).

@cbrnr
Copy link
Contributor

cbrnr commented Aug 26, 2019

PPS: Here's the code snippet I used (copied from your screenshots):

import os
import matplotlib.pyplot as plt
import mne


plt.get_backend()
sample_data_folder = mne.datasets.sample.data_path()
sample_data_raw_file = os.path.join(sample_data_folder, 'MEG', 'sample',
                                    'sample_audvis_raw.fif')
raw = mne.io.read_raw_fif(sample_data_raw_file)
raw.crop(0, 60).load_data()
fig = raw.plot()  # works
fig.subplots_adjust(top=0.9)  # works
# now resize the window - destroys figure layout

@drammock
Copy link
Member Author

@cbrnr @larsoner I've pushed a fix that I think will work. Please give it a try. (shout out to @ktavabi for letting me use his machine to debug).

@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2019

This pull request introduces 3 alerts when merging 2f1b9b6 into b7dd664 - view on LGTM.com

new alerts:

  • 3 for Unhashable object hashed

@cbrnr
Copy link
Contributor

cbrnr commented Aug 27, 2019

Nice! I can confirm that this now works with all combinations of osx/qt5 backends and retina/non-retina resolutions for me.

If the LGTM warnings are fixed (by e.g. using tuples instead of lists), +1 for merge!

@drammock
Copy link
Member Author

@cbrnr those LGTM alerts are unrelated to this PR, and I would consider them false alarms. Moreover I don't think it will work to just change them to tuples. Two of the lists it's complaining about are being used to index numpy arrays, and lists/tuples are treated differently in that context. The third one is an instance of slice that is used to index a list. Frankly I'm not sure why those objects even need to be hashable in that context.

@drammock
Copy link
Member Author

@agramfort
Copy link
Member

agramfort commented Aug 27, 2019 via email

@cbrnr cbrnr merged commit 93c7571 into mne-tools:master Aug 27, 2019
@cbrnr
Copy link
Contributor

cbrnr commented Aug 27, 2019

Thanks @drammock!

@drammock drammock deleted the raw_subplots_adjust branch August 27, 2019 18:30
@larsoner
Copy link
Member

BTW @drammock if you ever need to debug HiDPI on macOS on a non-HiDPI display, you can enable (usually ugly) HiDPI resolutions typically hidden from the user with something like:

$ sudo defaults write /Library/Preferences/com.apple.windowserver.plist DisplayResolutionEnabled -bool true

It's also possible clicking the "scaled" button while holding Alt/Option might do it (or be necessary).

I did something like this a couple of years ago when I only had access to a non-HiDPI macOS display and it worked. So hopefully next time you won't need to borrow @ktavabi's machine but any macOS one (where the owner doesn't mind some option tweaking) should work :)

alexrockhill pushed a commit to alexrockhill/mne-python that referenced this pull request Oct 1, 2019
…plot_sources() (mne-tools#6670)

* make raw.plot() compatible with subplots_adjust()

* tweak tutorial to test subplots_adjust

* fix doctest

* minor refactor

* subplots_adjust works for epochs.plot()

* works with ICA

* delete callback functions

* refactor/DRY

* fix unused imports

* fix pydocstyle

* fix for older versions of matplotlib

* fix for hidpi screens

* fix: add back resize listener

* fix: add callback to epochs

* add callback to ICA

* fix wrapping

* refactor: DRY

* fix for Retina displays
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.

BUG, VIZ: can't use fig.subplots_adjust on raw.plot figure

4 participants