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

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

Merged
merged 9 commits into from
Jan 17, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Jan 16, 2020

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
Copy link
Contributor Author

GuillaumeFavelier commented Jan 16, 2020

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

codecov bot commented Jan 16, 2020

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
Member

agramfort commented Jan 16, 2020 via email

@GuillaumeFavelier
Copy link
Contributor Author

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

@GuillaumeFavelier
Copy link
Contributor Author

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
Member

Agreed

@GuillaumeFavelier
Copy link
Contributor Author

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*

@GuillaumeFavelier
Copy link
Contributor Author

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
Member

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
Contributor Author

GuillaumeFavelier commented Jan 16, 2020

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
Contributor Author

This was a big refactor but now playback works too:

output

@larsoner
Copy link
Member

Beautiful!

@GuillaumeFavelier
Copy link
Contributor Author

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

@larsoner
Copy link
Member

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
Contributor Author

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

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Jan 16, 2020

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

Copy link
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

@larsoner
Copy link
Member

Works beautifully, awesome @GuillaumeFavelier !

@larsoner larsoner merged commit 2b579cb into mne-tools:master Jan 17, 2020
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* 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
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants