Skip to content

MRG: Link cameras in _LinkViewer#7962

Merged
larsoner merged 14 commits intomne-tools:masterfrom
GuillaumeFavelier:linkviewer_camera
Jul 23, 2020
Merged

MRG: Link cameras in _LinkViewer#7962
larsoner merged 14 commits intomne-tools:masterfrom
GuillaumeFavelier:linkviewer_camera

Conversation

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Jul 6, 2020 •
edited
Loading

This PR adds parameters to link_brains() to decide what to link: time controller and/or camera controls.

ToDo

  • Fix camera update delay
  • Update testing in test_3d.py
  • Rename the time_viewer used as reference into leader
  • Check stc time infos
  • Link time course canvas

It's an item of #7162
Related to #7959

@GuillaumeFavelier GuillaumeFavelier self-assigned this Jul 6, 2020
@GuillaumeFavelier GuillaumeFavelier changed the title Link cameras in _LinkViewer WIP: Link cameras in _LinkViewer Jul 6, 2020
@agramfort
Copy link
Copy Markdown
Member

agramfort commented Jul 6, 2020

@bloyl can you give it a try?

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Jul 6, 2020
Closed
86 tasks
larsoner
larsoner reviewed Jul 7, 2020
View reviewed changes
@bloyl
Copy link
Copy Markdown
Contributor

bloyl commented Jul 20, 2020

Sorry for the delay in testing this.

This is pretty cool and does what I'd like.

A couple of things I noticed.

  1. The camera doesn't force an update on the other renderer so things get laggy. I assume this is what you mean by Fix camera update delay in the description. but thought I'd mention it.
  2. The time linking only works on the slider. if you click in the time course plot only the one brain updates.
  3. You are currently allowed to time link brains that have different time axis. but since the linking is on time index this causes things to get out of sync.

My suggestion for point 3 would be use the actual time as opposed to the index to do the linking. Additionally I would add a time='auto' option as the default. this would use time=True if the linked brain objects all have the time array and false if they don't, supporting both linked multiple evoked STCs and unlinked stcs (say evoked and a PSD) with the default. Additonally if a user had evoked stcs with different time axis that were actually comparable (maybe historic data or something) they could force time=True to still have the linking.

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jul 21, 2020

The camera doesn't force an update on the other renderer so things get laggy.

The cameras are linked but the event "the camera has been modified" is not captured simply because I don't know how to do that. And since #7517, the plotter is not updated automatically. I see 2 ways to fix this. Either capture the vtk event (interactor, camera...) or allow automatic update again.

The time linking only works on the slider

Nice catch! I'll take care of this :)

You are currently allowed to time link brains that have different time axis.

Hm, this is not intended... I have to check the time axes somehow. @larsoner, @agramfort any idea on how to deal with this?

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Jul 21, 2020 via email

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jul 21, 2020

  1. and 3) should be fixed by now. I'll investigate 1) and see if I can capture the right VTK event to update the plotters. Otherwise, I'll have to re-enable automatic update to mitigate.

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jul 21, 2020

It turns out, the camera ModifiedEvent does the trick! I invite you to try it out @bloyl :)

output

Reference: http://vtk.1045678.n5.nabble.com/Vtk-Camera-Postion-Event-td5727757.html

@GuillaumeFavelier GuillaumeFavelier changed the title WIP: Link cameras in _LinkViewer MRG: Link cameras in _LinkViewer Jul 21, 2020
bloyl
bloyl reviewed Jul 21, 2020
View reviewed changes
mne/viz/_brain/_timeviewer.py
Comment on lines +1207 to +1213
# check time infos
brain_times = [brain._times for brain in brains]
if not all(len(x) == len(brain_times[0]) for x in brain_times):
time = False
if not all(all(np.equal(x, brain_times[0])) for x in brain_times):
time = False

Copy link
Copy Markdown
Contributor

@bloyl bloyl Jul 21, 2020

Choose a reason for hiding this comment

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

This second check throws ValueError: operands could not be broadcast if the brain_times are of different lengths and more than one.

why not only do the check if you have to (time == True)

        if time and not all(len(x) == len(brain_times[0]) for x in brain_times):
            time = False
        if time and not all(all(np.equal(x, brain_times[0])) for x in brain_times):
            time = False

to test just link a plot of an evoked stc with a cropped copy of the same stc

@bloyl
Copy link
Copy Markdown
Contributor

bloyl commented Jul 21, 2020

Other than my comment above this is working for me.

Additional question, how did you make the 4 panel figure on the right?

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jul 21, 2020

how did you make the 4 panel figure on the right?

I use the following parameters: hemi='split', views=['lat', 'med']

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Jul 21, 2020

Feel free to have a look or even test it @agramfort, @larsoner

larsoner
larsoner reviewed Jul 21, 2020
View reviewed changes
@GuillaumeFavelier @larsoner
Use np.allclose
3eb4d50 
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@larsoner larsoner merged commit 6e5beb6 into mne-tools:master Jul 23, 2020
@GuillaumeFavelier GuillaumeFavelier deleted the linkviewer_camera branch July 23, 2020 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@larsoner larsoner larsoner left review comments

@agramfort agramfort agramfort approved these changes

+1 more reviewer

@bloyl bloyl bloyl left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@GuillaumeFavelier GuillaumeFavelier

Labels

VIZ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@GuillaumeFavelier @agramfort @bloyl @larsoner