Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Fix concat on Windows #9351

Merged
merged 3 commits into from
Apr 27, 2021
Merged

BUG: Fix concat on Windows #9351

merged 3 commits into from
Apr 27, 2021

Conversation

larsoner
Copy link
Member

Closes #9336

@crsegerie can you see if this fixes your problem?

I was able to replicate on Windows. Basically I think the problem was that after our DPI calculations, the resulting size was something like 1406.99999999 instead of 1407, so MPL made something with only 1406 pixels instead of 1407 and this broke things. We could try tweaking our DPI calculations, but it seems safer just to output a PNG then read it back, as this has the dimensions, then make use of our smart concat function to take care of any difference in dimension.

Okay with you @GuillaumeFavelier ?

@larsoner larsoner added this to the 0.23 milestone Apr 26, 2021
@crsegerie
Copy link

Yeah, this solves my problem! Thank you!

@agramfort
Copy link
Member

this PR breaks the screenshots for me on my mac. I get screenshots like this now

toto2

@GuillaumeFavelier
Copy link
Contributor

Thanks for pointing it out @agramfort because test_brain_screenshot is skipped for the macos/conda job:

mne/viz/_brain/tests/test_brain.py::test_brain_screenshot[pyvista] SKIPPED [ 95%]
...
SKIPPED [2] mne/viz/_brain/tests/test_brain.py:351: Unreliable/segfault on macOS CI

https://github.com/mne-tools/mne-python/pull/9351/checks?check_run_id=2440366851#step:12:2839
https://github.com/mne-tools/mne-python/pull/9351/checks?check_run_id=2440366851#step:12:3040

@GuillaumeFavelier
Copy link
Contributor

@larsoner what do you think about trying the alternative calculation only when the reshape fails?

@larsoner
Copy link
Member Author

Thanks for pointing it out @agramfort because test_brain_screenshot is skipped for the macos/conda job:

I think it's just a 0-1 vs 0-255 mapping problem, I'll test locally and push a fix

@larsoner what do you think about trying the alternative calculation only when the reshape fails?

I'd rather not have two code paths. If we're forced to go back to raw mode, we need to figure out somehow what shape the output actually needs to be. But like I mention above I doubt it will be necessary

@larsoner
Copy link
Member Author

larsoner commented Apr 27, 2021

This code:

Code
import os.path as op
import mne
from mne.minimum_norm import read_inverse_operator, apply_inverse

# Set dir
data_path = mne.datasets.sample.data_path()
subject = 'sample'
data_dir = op.join(data_path, 'MEG', subject)
subjects_dir = op.join(data_path, 'subjects')
fname_evoked = data_dir + '/sample_audvis-ave.fif'
condition = 'Left Auditory'
evoked = mne.read_evokeds(fname_evoked, condition=condition,
                          baseline=(None, 0))
inv = read_inverse_operator(
    data_path + '/MEG/sample/sample_audvis-meg-oct-6-meg-inv.fif')
stc_vec = apply_inverse(evoked, inv, pick_ori='vector')
with mne.viz.use_3d_backend('pyvista'):
    brain = stc_vec.plot(hemi='both', views=['lat', 'med'],
                         initial_time=0.1, subjects_dir=subjects_dir)
import matplotlib.pyplot as plt
plt.figimage(brain.screenshot(time_viewer=True), resize=True)

Gives on main:

Screen Shot 2021-04-27 at 7 11 06 AM

On what @agramfort tested you can see two bugs -- one I introduced and another involving bgcolor that is on main but not seen there in this example (white line running vertically along the lower left part of the image):

Screen Shot 2021-04-27 at 7 11 31 AM

Pushed a commit to fix both problems:

Screen Shot 2021-04-27 at 7 13 54 AM

Clearly the image of the brain is wrong, and the matplotlib plot is not the right size either. But these are separate issues on main, too, so let's fix them separately.

@GuillaumeFavelier do you want to try my code snippet to see if you see the same problems on your system?

@larsoner
Copy link
Member Author

Okay the problem was really with my figimage call, I needed resize=True (edited now above) and I get something correct:

Screenshot from 2021-04-27 07-42-38

But there is a problem with the update of the display in that the views don't respond to the fact that the matplotlib traces have forced them to resize:

Screenshot from 2021-04-27 07-42-43

@GuillaumeFavelier
Copy link
Contributor

I would have to switch to KDE for a better test but on i3 (which maximizes the new windows by default) I think I can reproduce:

image

When I leave the maximized mode:

image

Could it be yet another process events?

@larsoner
Copy link
Member Author

For me, deleting the self._interactor.resize(ren_sz.width(), ren_sz.height()) in _window_ensure_minimum_sizes makes it look correct. Going to see what it breaks on Linux and macOS locally

@larsoner
Copy link
Member Author

... actually it doesn't make it work, it makes it look not cut off, which is good, but it's the wrong size, which is bad. I'll see if I can figure out what to do to make it work

@larsoner
Copy link
Member Author

Okay should be fixed, can you review and test locally @GuillaumeFavelier ?

Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier left a comment

Choose a reason for hiding this comment

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

For me, everything works locally and the changes look good.

@larsoner larsoner merged commit 255092b into mne-tools:main Apr 27, 2021
@larsoner larsoner deleted the concat branch April 27, 2021 13:40
@hoechenberger
Copy link
Member

Thanks @larsoner!!

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.

brain.screenshot(time_viewer=True) crash
5 participants