MRG, ENH: Add a screenshot button to the notebook 3d backend #8708

GuillaumeFavelier · 2021-01-07T14:44:11Z

There is actually a button to save effectively plus a text field to change the file name:

I also switched to icon + tooltip because it was a bit cramped with all those descriptions:

It's an item of #8704.

agramfort · 2021-01-07T15:05:13Z

did you consider using a dialog box to be able to save the screenshot where you want on disk?

see eg https://pypi.org/project/ipyfilechooser/

just a quick google search...

GuillaumeFavelier · 2021-01-07T15:08:12Z

I considered it but this solution is simple and does not add any dependency.

larsoner · 2021-01-07T18:45:15Z

Would be good to add some "interaction" tests in test_notebook if possible. This seems like a good PR where this could be investigated and done.

GuillaumeFavelier · 2021-01-08T10:55:23Z

After e3198f7, there is a tool bar for standard 3d plots too:

GuillaumeFavelier · 2021-01-08T11:48:17Z

Known issue

When I resize the page in my web browser, the 3d viewer and the matplotlib widget don't scale the same way leading to issues when I save the screenshot. For example 80%:

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
~/source/mne-python/mne/viz/backends/_notebook.py in <lambda>(x)
     28         from ipywidgets import Button
     29         button = Button(tooltip=desc, icon=icon_name)
---> 30         button.on_click(lambda x: func())
     31         return button
     32 

~/source/mne-python/mne/viz/_brain/_brain.py in _screenshot(self)
   1222             fname = self._renderer.screenshot_filename \
   1223                 if len(fname) == 0 else fname
-> 1224             img = self.screenshot(fname, time_viewer=True)
   1225             Image.fromarray(img).save(fname)
   1226         else:

~/source/mne-python/mne/viz/_brain/_brain.py in screenshot(self, mode, time_viewer)
   2634                 start = delta // 2
   2635                 trace_img = trace_img[:, start:start + img.shape[1]]
-> 2636             img = np.concatenate([img, trace_img], axis=0)
   2637         return img
   2638 

<__array_function__ internals> in concatenate(*args, **kwargs)

ValueError: all the input array dimensions for the concatenation axis must match exactly, but along dimension 1, the array at index 0 has size 800 and the array at index 1 has size 799

When everything works fine at 100%:

GuillaumeFavelier · 2021-01-08T11:52:08Z

If this is not a big issue, we can leave this as is otherwise I can mitigate by using the standard screenshot function but the matplotlib figure won't be present.

larsoner · 2021-01-08T12:06:01Z

I would write a mne.viz.utils.concatenate_images that allows you to pass a list of array/images and concatenates them vertically (or horizontally, could be an axis argument that must be 0 or 1) padding and centering the smaller one(s) as needed. This will require a bgcolor argument, which could be 'auto' to use for example the most common color amongst all 4 corner pixels across all images or something.

I've written something like this and needed it multiple times for paper publications and the like so making this public would be useful. We can probably even use it in the publication figure example.

agramfort · 2021-01-09T11:13:06Z

here is the screenshot I get on my system :(

anyone else can try? is it just me?

larsoner · 2021-01-09T13:05:14Z

That looks like a hidpi issue

GuillaumeFavelier · 2021-01-09T13:10:37Z

That looks like a hidpi issue

Oh no, not again... 😟

agramfort · 2021-01-09T13:14:00Z

I'm going to give you a mac so you can test this :)

mne/viz/backends/_pyvista.py

mne/label.py

mne/viz/_brain/_brain.py

mne/datasets/utils.py

larsoner · 2021-01-19T18:49:12Z

Also at some point we must have broken screenshot size on macOS, on the test locally I get:

    assert img_nv.shape == want
E   assert (558, 600, 3) == (600, 600, 3)

So even without the time viewer the correct size is not preserved. :(

GuillaumeFavelier · 2021-01-20T12:03:02Z

Also at some point we must have broken screenshot size on macOS

Does it mean that ensure_minimum_sizes() does not work on MacOS?

larsoner · 2021-01-20T12:08:38Z

Does it mean that ensure_minimum_sizes() does not work on MacOS?

At least it doesn't on my macOS system (with HiDPI), and apparently Azure macOS (no HiDPI) agrees:

E     - (300, 300, 3)
E     + (284, 300, 3)

I can git bisect to see which PR broke things. I think it worked a while back at least, but maybe it never did. In a worst case we can add an extra vertical tolerance to this PR and fix the sizing issue elsewhere.

larsoner · 2021-01-20T12:24:45Z

Looks like I broke it back in #8082. Do you want to try to fix it? You probably need a macOS system to be able to try different things out effectively so if you don't have one, I can do it

GuillaumeFavelier · 2021-01-20T14:48:33Z

I looked at #8082 again and I did not see anything obvious except maybe the changes of ensure_minimum_sizes(). The following is not present anymore:

            # 5. Resize the window (again!) to the correct size
            #    (not sure why, but this is required on macOS at least)
            self.plotter.window_size = (sz.width(), sz.height())

probably need a macOS system

You are right. I'll push one last commit but I can't try locally so you can go for it if you have the time

GuillaumeFavelier · 2021-01-20T15:45:03Z

I got no luck with 930fdcf

larsoner

@GuillaumeFavelier feel free to take over again based on my comments, but in theory in the meantime the CIs should be green

larsoner · 2021-01-20T20:36:41Z

mne/viz/_brain/_brain.py

+            fig = self.mpl_canvas.fig
+            with BytesIO() as output:
+                # Need to pass dpi here so it uses the physical (HiDPI) DPI
+                # rather than logical DPI when saving
+                fig.savefig(output, dpi=fig.get_dpi(), format='raw')
+                output.seek(0)
+                trace_img = np.reshape(
+                    np.frombuffer(output.getvalue(), dtype=np.uint8),
+                    newshape=(-1, img.shape[1], 4))[:, :, :3]
+            img = concatenate_images(
+                [img, trace_img], bgcolor=self._brain_color[:3])


@GuillaumeFavelier this seems to work for both code paths now on Linux and macOS for me (notebook and standard renderer)

This is awesome news!

larsoner · 2021-01-20T20:40:27Z

mne/viz/_brain/tests/test.ipynb

+    "            number_of_buttons += 1\n",
+    "    assert number_of_buttons == total_number_of_buttons\n",
+    "    img_nv = brain.screenshot()\n",
+    "    assert img_nv.shape == (300, 300, 3), img_nv.shape\n",


Notebook always renders non-HiDPI. At some point we should look into how to enable HiDPI support in the notebook -- basically we need the background renderer to be ratio (usually 2) times the size, and we need to tell it to display the image in native resolution (rather than treating physical and logical pixels the same). I suspect this is supported at some level in browsers, but TBD whether ipyvtk_simple allows setting this display resolution factor (i.e., to make images that are 600x600 pixels show up in a 300x300 logical pixel HTML element). Could be a cool PR there. Then we'd need a way to probe the physical-to-logical pixel ratio. Ideally this would come from the notebook but if not, we can quickly import Qt and query the screens and take a best guess...

larsoner · 2021-01-20T20:41:23Z

mne/viz/_brain/tests/test.ipynb

+    "    img_v = brain.screenshot(time_viewer=True)\n",
+    "    assert img_v.shape[1:] == (300, 3), img_v.shape\n",
+    "    # XXX This rtol is not very good, ideally would be zero\n",
+    "    assert_allclose(img_v.shape[0], img_nv.shape[0] * 1.25, err_msg=img_nv.shape, rtol=0.1)\n",


HiDPI aside, on Linux I was getting values that were off by ~7%. It would be good if this were as good as the non-notebook backend (which uses atol=3 instead of the rtol=0.1 here).

larsoner · 2021-01-20T20:41:46Z

mne/viz/_brain/tests/test.ipynb

+    "    finally:\n",
+    "        plt.interactive(old)\n",
+    "\n",
+    "with interactive(False):\n",


I'm not sure how critical the iteration over interactive levels was so I killed it to save time now that this isn't marked slowtest.

mne/viz/_brain/tests/test.ipynb

agramfort · 2021-01-21T10:15:20Z

😱

larsoner · 2021-01-21T13:26:24Z

@agramfort what is your mne.sys_info? I can't replicate in my env in pure Python or Jupyter. I wonder if it's because you have PyQt >= 5.14 on macOS (which we don't do in our environment.yml or requirements.txt), where VTK doesn't do HiDPI, but matplotlib probably does... it would explain this monstrosity. :)

I'll push a fix to deal with it, but shame on you for having a non-standard env...

larsoner · 2021-01-21T13:39:37Z

@agramfort feel free to try this, works for me on macOS in pure Python and Jupyter with PyQt5 < 5.14 (5.13.2) and >= 5.14 (5.15.2)

GuillaumeFavelier · 2021-01-21T14:04:49Z

mne/viz/_brain/_brain.py

+                # (e.g., macOS w/Qt 5.14+ and VTK9) then things won't work,
+                # so let's just calculate the DPI we need to get
+                # the correct size output based on the widths being equal
+                dpi = img.shape[1] / fig.get_size_inches()[0]


Thank you for this @larsoner

It also works for me on Linux in pure Python and Jupyter

agramfort · 2021-01-21T15:11:16Z

thanks @GuillaumeFavelier success !!!

GuillaumeFavelier added 3 commits January 7, 2021 15:31

Add screenshot button

d236c1c

Ensure valid filename

95ab3ec

Make the name shorter

82076aa

GuillaumeFavelier added the ENH label Jan 7, 2021

GuillaumeFavelier self-assigned this Jan 7, 2021

Use Brain screenshot

07be3c7

agramfort approved these changes Jan 7, 2021

View reviewed changes

GuillaumeFavelier changed the title ~~Add a screenshot button to the notebook 3d backend~~ WIP: Add a screenshot button to the notebook 3d backend Jan 8, 2021

GuillaumeFavelier mentioned this pull request Jan 8, 2021

ENH: Notebook 3d backend improvements (ipyvtklink) #8704

Closed

11 tasks

GuillaumeFavelier added 4 commits January 8, 2021 10:44

Click on all buttons

a5cbed0

Count the buttons too

a284f43

Add a tool bar to the standard _Renderer

e3198f7

Improve testing of standard _Renderer

ed67a41

DRY a little bit

1a13ea7

GuillaumeFavelier added 4 commits January 8, 2021 16:49

Use concatenate_images

0fd9744

Make it shorter and more complicated

4dd9fc9

Fix style

4073c1d

Add centered parameter

11663a2

Merge branch 'master' into enh/notebook_screenshot

9ca7736

larsoner requested changes Jan 19, 2021

View reviewed changes

mne/viz/backends/_pyvista.py Outdated Show resolved Hide resolved

mne/label.py Show resolved Hide resolved

mne/viz/_brain/_brain.py Show resolved Hide resolved

larsoner reviewed Jan 19, 2021

View reviewed changes

mne/datasets/utils.py Show resolved Hide resolved

BUG: More explicit height

031d4dc

GuillaumeFavelier added 2 commits January 20, 2021 10:41

Fix dangling objects issue

54fe438

Change order

972b8a8

Try mne-tools#8082

930fdcf

larsoner added 5 commits January 20, 2021 12:43

FIX: Fix sizing

3c0b083

FIX: Use concatenate_images

2c17547

FIX: dtype

34c804c

MAINT: Notebook test

8e927e7

FIX: Flake

5d34669

larsoner reviewed Jan 20, 2021

View reviewed changes

Speed up test.ipynb

1e28b73

GuillaumeFavelier changed the title ~~MRG: Add a screenshot button to the notebook 3d backend~~ WIP: Add a screenshot button to the notebook 3d backend Jan 21, 2021

FIX: Bad Qt/VTK combo

2a96cdd

larsoner changed the title ~~WIP: Add a screenshot button to the notebook 3d backend~~ MRG, ENH: Add a screenshot button to the notebook 3d backend Jan 21, 2021

larsoner approved these changes Jan 21, 2021

View reviewed changes

GuillaumeFavelier commented Jan 21, 2021

View reviewed changes

agramfort merged commit 72c8f61 into mne-tools:master Jan 21, 2021

GuillaumeFavelier deleted the enh/notebook_screenshot branch January 21, 2021 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MRG, ENH: Add a screenshot button to the notebook 3d backend #8708

MRG, ENH: Add a screenshot button to the notebook 3d backend #8708

GuillaumeFavelier commented Jan 7, 2021 •

edited

Loading

agramfort commented Jan 7, 2021

GuillaumeFavelier commented Jan 7, 2021 •

edited

Loading

larsoner commented Jan 7, 2021

GuillaumeFavelier commented Jan 8, 2021

GuillaumeFavelier commented Jan 8, 2021

GuillaumeFavelier commented Jan 8, 2021

larsoner commented Jan 8, 2021

agramfort commented Jan 9, 2021

larsoner commented Jan 9, 2021

GuillaumeFavelier commented Jan 9, 2021

agramfort commented Jan 9, 2021 via email

larsoner commented Jan 19, 2021

GuillaumeFavelier commented Jan 20, 2021

larsoner commented Jan 20, 2021

larsoner commented Jan 20, 2021

GuillaumeFavelier commented Jan 20, 2021

GuillaumeFavelier commented Jan 20, 2021

larsoner left a comment

larsoner Jan 20, 2021

GuillaumeFavelier Jan 21, 2021

larsoner Jan 20, 2021

larsoner Jan 20, 2021

larsoner Jan 20, 2021

agramfort commented Jan 21, 2021

larsoner commented Jan 21, 2021

larsoner commented Jan 21, 2021

GuillaumeFavelier Jan 21, 2021

GuillaumeFavelier Jan 21, 2021

agramfort commented Jan 21, 2021

MRG, ENH: Add a screenshot button to the notebook 3d backend #8708

MRG, ENH: Add a screenshot button to the notebook 3d backend #8708

Conversation

GuillaumeFavelier commented Jan 7, 2021 • edited Loading

agramfort commented Jan 7, 2021

GuillaumeFavelier commented Jan 7, 2021 • edited Loading

larsoner commented Jan 7, 2021

GuillaumeFavelier commented Jan 8, 2021

GuillaumeFavelier commented Jan 8, 2021

GuillaumeFavelier commented Jan 8, 2021

larsoner commented Jan 8, 2021

agramfort commented Jan 9, 2021

larsoner commented Jan 9, 2021

GuillaumeFavelier commented Jan 9, 2021

agramfort commented Jan 9, 2021 via email

larsoner commented Jan 19, 2021

GuillaumeFavelier commented Jan 20, 2021

larsoner commented Jan 20, 2021

larsoner commented Jan 20, 2021

GuillaumeFavelier commented Jan 20, 2021

GuillaumeFavelier commented Jan 20, 2021

larsoner left a comment

Choose a reason for hiding this comment

larsoner Jan 20, 2021

Choose a reason for hiding this comment

GuillaumeFavelier Jan 21, 2021

Choose a reason for hiding this comment

larsoner Jan 20, 2021

Choose a reason for hiding this comment

larsoner Jan 20, 2021

Choose a reason for hiding this comment

larsoner Jan 20, 2021

Choose a reason for hiding this comment

agramfort commented Jan 21, 2021

larsoner commented Jan 21, 2021

larsoner commented Jan 21, 2021

GuillaumeFavelier Jan 21, 2021

Choose a reason for hiding this comment

GuillaumeFavelier Jan 21, 2021

Choose a reason for hiding this comment

agramfort commented Jan 21, 2021

GuillaumeFavelier commented Jan 7, 2021 •

edited

Loading

GuillaumeFavelier commented Jan 7, 2021 •

edited

Loading