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: Link _TimeViewer time slider with vertical time line #7423

Merged
merged 8 commits into from
Mar 13, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

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

@codecov
Copy link

codecov bot commented Mar 11, 2020

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     

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

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
Member

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

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

Okay, might not be worth it then...

@GuillaumeFavelier
Copy link
Contributor Author

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
Member

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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants