Skip to content

Fix window name in _TimeViewer#7382

Merged
agramfort merged 6 commits intomne-tools:masterfrom
GuillaumeFavelier:time_viewer_window_title
Mar 5, 2020
Merged

Fix window name in _TimeViewer#7382
agramfort merged 6 commits intomne-tools:masterfrom
GuillaumeFavelier:time_viewer_window_title

Conversation

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Mar 4, 2020

This PR fixes the window title of _TimeViewer. The diff is very short, the title parameter was not passed to _get_renderer().

It's an item of #7162

@GuillaumeFavelier GuillaumeFavelier self-assigned this Mar 4, 2020
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Mar 4, 2020

Can you check to make sure that however PyVista handles the name here, it does not make it that passing the same name/title twice leads to the same figure handle? See for example nipy/PySurfer@5aaf043 where if we did mlab.figure(title, ...) twice it would point to the same figure handle because of how mlab.figure works, similar to how doing fig1 = plt.figure(1); fig2 = plt.figure(1) will have fig1 and fig2 point to the same handle. This can be desirable in some situations but it's not good for our purposes in how we use the 3D plotters.

agramfort
agramfort reviewed Mar 4, 2020
View reviewed changes
mne/viz/_brain/_brain.py Outdated
if figure is not None and not isinstance(figure, int):
_check_3d_figure(figure)
self._renderer = _get_renderer(size=fig_size, bgcolor=background,
self._renderer = _get_renderer(name=title, size=fig_size,
Copy link
Copy Markdown
Member

@agramfort agramfort Mar 4, 2020

Choose a reason for hiding this comment

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

name=title raises some skepticism but I guess it's internal code so...

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Mar 4, 2020 •
edited
Loading

fig is a renderer mechanic, I implemented it in mne/viz/backends/_pyvista.py to reproduce the way mayavi handle figures. I thought it was a required feature.

EDIT: fig reproduces mayavi behaviour, not name

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Mar 4, 2020

See:

mne-python/mne/viz/backends/_pyvista.py

Lines 122 to 133 in c4e9cbf

if isinstance(fig, int):
saved_fig = _FIGURES.get(fig)
# Restore only active plotter
if saved_fig is not None and saved_fig.is_active():
self.figure = saved_fig
else:
self.figure = figure
_FIGURES[fig] = self.figure
elif fig is None:
self.figure = figure
else:
self.figure = fig

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Mar 4, 2020

Okay looks good

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Mar 4, 2020

I discussed offline with @agramfort and I think usingname=title can be misleading.

I think using set_3d_title is a better idea now.

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Mar 4, 2020

Sorry for the confusion.

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Mar 4, 2020 via email

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 4, 2020 •
edited
Loading

Codecov Report

Merging #7382 into master will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7382      +/-   ##
==========================================
+ Coverage   89.91%   90.06%   +0.14%     
==========================================
  Files         453      453              
  Lines       82041    82795     +754     
  Branches    12999    13222     +223     
==========================================
+ Hits        73767    74569     +802     
+ Misses       5445     5394      -51     
- Partials     2829     2832       +3

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Mar 4, 2020

This is a now different approach: _set_3d_title() will add a text label in the scene.

@larsoner larsoner added this to the 0.20 milestone Mar 4, 2020
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Mar 4, 2020

Failure is real:

https://travis-ci.org/mne-tools/mne-python/jobs/658249771

@agramfort agramfort merged commit bbd17dd into mne-tools:master Mar 5, 2020
@agramfort
Copy link
Copy Markdown
Member

agramfort commented Mar 5, 2020

thx @GuillaumeFavelier !

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Mar 5, 2020
Closed
86 tasks
@GuillaumeFavelier GuillaumeFavelier deleted the time_viewer_window_title branch March 6, 2020 08:27
@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Mar 6, 2020
Merged
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
@agramfort @AdoNunes
Fix window name in _TimeViewer (mne-tools#7382)
531cca0 
* Fix window name

* Revert "Fix window name"

This reverts commit c4e9cbf.

* Use _set_3d_title to set the title

* Improve stability

* Use unified renderer API

* Fix parameter name
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
@agramfort @AdoNunes
Fix window name in _TimeViewer (mne-tools#7382)
57dae03 
* Fix window name

* Revert "Fix window name"

This reverts commit c4e9cbf.

* Use _set_3d_title to set the title

* Improve stability

* Use unified renderer API

* Fix parameter name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agramfort agramfort agramfort approved these changes

@larsoner larsoner larsoner approved these changes

Assignees

@GuillaumeFavelier GuillaumeFavelier

Labels

VIZ

Projects

None yet

Milestone

0.20

Development

Successfully merging this pull request may close these issues.

3 participants

@GuillaumeFavelier @larsoner @agramfort