Skip to content

MRG: Link _TimeViewer time slider with vertical time line#7423

Merged
larsoner merged 8 commits intomne-tools:masterfrom
GuillaumeFavelier:time_viewer_time_line
Mar 13, 2020
Merged

MRG: Link _TimeViewer time slider with vertical time line#7423
larsoner merged 8 commits intomne-tools:masterfrom
GuillaumeFavelier:time_viewer_time_line

Conversation

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Mar 11, 2020

This PR adds a vertical line to indicate the current time on the matplotlib canvas.

I also did quite the refactor there as _TimeViewer.__init__ was getting too long.

output

It's an item of #7162

@GuillaumeFavelier GuillaumeFavelier self-assigned this Mar 11, 2020
larsoner
larsoner reviewed Mar 11, 2020
View reviewed changes
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 11, 2020 •
edited
Loading

Codecov Report

Merging #7423 into master will increase coverage by 4.45%.
The diff coverage is 75.23%.

@@            Coverage Diff             @@
##           master    #7423      +/-   ##
==========================================
+ Coverage   85.61%   90.07%   +4.45%     
==========================================
  Files         454      454              
  Lines       82227    82358     +131     
  Branches    13003    13025      +22     
==========================================
+ Hits        70397    74182    +3785     
+ Misses       9162     5351    -3811     
- Partials     2668     2825     +157

larsoner
larsoner approved these changes Mar 11, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Pushed a commit to enable bidirectional control (clicking in the MPL axis will change the brain and sliders). @GuillaumeFavelier I don't know if it was the cleanest way so feel free to take a look and fix if need be.

Now this works the way I would expect so LGTM +1 for merge from me

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

GuillaumeFavelier commented Mar 12, 2020

feel free to take a look and fix if need be.

Could not have done better.

I just added to_time_index() to _Brain

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Mar 12, 2020

@GuillaumeFavelier can we make that private and also get rid of index_for_time? Things like index_for_time seem like cruft left over from PySurfer that we don't want in the public interface of our new Brain class.

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Mar 12, 2020

@larsoner there are multiple places where I consider that time_idx is int (which was guaranteed by index_for_time with the default parameters). This might take more time than expected to refactor.

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Mar 12, 2020

Okay, might not be worth it then...

@GuillaumeFavelier
Copy link
Copy Markdown
Contributor Author

GuillaumeFavelier commented Mar 12, 2020

I took care of it in the latest commit 363e81f, what do you think?

@GuillaumeFavelier GuillaumeFavelier changed the title Link _TimeViewer time slider with vertical time line MRG: Link _TimeViewer time slider with vertical time line Mar 13, 2020
@larsoner larsoner merged commit 0083c2b into mne-tools:master Mar 13, 2020
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Mar 13, 2020

Thanks @GuillaumeFavelier

@larsoner larsoner added this to the 0.20 milestone Mar 13, 2020
@GuillaumeFavelier GuillaumeFavelier deleted the time_viewer_time_line branch June 11, 2020 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@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.

2 participants

@GuillaumeFavelier @larsoner