Skip to content

MRG: Add support for hemi=split in time_viewer#7219

Merged
larsoner merged 9 commits intomne-tools:masterfrom
GuillaumeFavelier:time_viewer_hemi_split
Jan 17, 2020
Merged

MRG: Add support for hemi=split in time_viewer#7219
larsoner merged 9 commits intomne-tools:masterfrom
GuillaumeFavelier:time_viewer_hemi_split

Conversation

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Jan 16, 2020 •
edited
Loading

This PR starts the work on hemi=split. Let's go incrementally on this one. For now I put all the controls on the first view.

Using plot_visualize_stc.py with hemi=split:

2020-01-16_1920x1080

The zoom between the views is not linked, that would be nice to have I think.

ToDo

  • Unify show_view() when hemi=split or when len(views)>1
  • Fix playback when len(views)>1
  • Fix default orientation value

It's an item of #7162

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Jan 16, 2020
Closed
86 tasks
@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jan 16, 2020 •
edited
Loading

Sadly with hemi=split and views=['lat', 'med'], the sliders got lost in the process:

2020-01-16_1920x1080

Just checking with time_viewer=False:

2020-01-16_1920x1080

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 16, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@b304337). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #7219   +/-   ##
=========================================
  Coverage          ?   89.77%           
=========================================
  Files             ?      445           
  Lines             ?    80046           
  Branches          ?    12802           
=========================================
  Hits              ?    71860           
  Misses            ?     5374           
  Partials          ?     2812

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Jan 16, 2020 via email

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jan 16, 2020

I'm not sure but it could be related to pyvista/pyvista#513

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jan 16, 2020

Or anyway I think we should wait for this issue to be solved in order to have full control on the sliders position between the views.

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jan 16, 2020

Agreed

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jan 16, 2020

Locally, following pyvista/pyvista#513 (comment) method, I managed to retrieve them:

2020-01-16_1920x1080

I can try to patch in _TimeViewer while waiting on a decision upstream.

Note: the views are not correct still but this is a different bug*

@larsoner larsoner mentioned this pull request Jan 16, 2020
Closed
@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jan 16, 2020

How show_view() should work in a hemi=split scenario? For now all the views are linked I guess it's not the most useful.

Should we duplicate the orientation slider in each view?

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jan 16, 2020

How show_view() should work in a hemi=split scenario?

My advice is to always look first at PySurfer to see what it does to at least get an idea:

https://pysurfer.github.io/generated/surfer.Brain.html#surfer.Brain.show_view

In this case it has row/col indices, which seems like a sensible API

Should we duplicate the orientation slider in each view?

That would be great!

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jan 16, 2020 •
edited
Loading

I think it's better now but the default values for the orientation slider are still not correct. Note that the interaction with the user remains correct:

2020-01-16_1920x1080

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jan 16, 2020

This was a big refactor but now playback works too:

output

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jan 16, 2020

Beautiful!

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jan 16, 2020

With this, the issue with the default values of the orientation sliders should be fixed.

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jan 16, 2020

Should we test/review/merge or wait for PyVista fixes upstream?

@GuillaumeFavelier GuillaumeFavelier changed the title WIP: Add support for hemi=split in time_viewer MRG: Add support for hemi=split in time_viewer Jan 16, 2020
@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jan 16, 2020

Feel free to review/test/merge this one @larsoner , @agramfort

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jan 16, 2020 •
edited
Loading

Oh and actually in your original comment @larsoner, you wanted to move some UI elements. If you test and you're not satisfied by the current setup, you can also let me know

agramfort
agramfort approved these changes Jan 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

works for me ! nice work @GuillaumeFavelier

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Jan 17, 2020
Merged
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Jan 17, 2020

Works beautifully, awesome @GuillaumeFavelier !

@larsoner larsoner merged commit 2b579cb into mne-tools:master Jan 17, 2020
@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Jan 17, 2020
Merged
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
@GuillaumeFavelier @AdoNunes
MRG: Add support for hemi=split in time_viewer (mne-tools#7219)
d6b786d 
* Start with all controls on the first view

* Fix show_view when hemi=split

* Mitigate slider disappearing if shape(1+,2)

* Dispatch an orientation slider per view

* Dispatch orientation slider to each view (not only row)

* Refactor brain entirely

* Fix default value for orientation slider

* Improve coverage
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
@GuillaumeFavelier @AdoNunes
MRG: Add support for hemi=split in time_viewer (mne-tools#7219)
77301e5 
* Start with all controls on the first view

* Fix show_view when hemi=split

* Mitigate slider disappearing if shape(1+,2)

* Dispatch an orientation slider per view

* Dispatch orientation slider to each view (not only row)

* Refactor brain entirely

* Fix default value for orientation slider

* Improve coverage
@GuillaumeFavelier GuillaumeFavelier deleted the time_viewer_hemi_split branch June 11, 2020 09:39
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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GuillaumeFavelier @agramfort @larsoner