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 nirs support to plot_evoked_topo #7408

Merged
merged 5 commits into from
Mar 9, 2020

Conversation

rob-luke
Copy link
Member

@rob-luke rob-luke commented Mar 9, 2020

Reference issue

Adding general NIRS support #7057

What does this implement/fix?

Allows plotting of evoked topo map for nirs data

@rob-luke rob-luke changed the title WIP: Add nirs support to plot_evoked_topo MRG: Add nirs support to plot_evoked_topo Mar 9, 2020
@rob-luke
Copy link
Member Author

rob-luke commented Mar 9, 2020

@larsoner could you please review. The function now behaves as I would expect. The CI errors seem unrelated to the code changes.

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.

@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #7408 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7408      +/-   ##
==========================================
+ Coverage   90.01%   90.03%   +0.01%     
==========================================
  Files         453      454       +1     
  Lines       82233    82214      -19     
  Branches    13012    13002      -10     
==========================================
- Hits        74023    74022       -1     
+ Misses       5384     5368      -16     
+ Partials     2826     2824       -2     

@rob-luke
Copy link
Member Author

rob-luke commented Mar 9, 2020

Sorry to modify this after you approved @agramfort
But I realised I had repeated some code that will probably be common as I add more nirs tests, so I have created a helper function to generate the data. Let me know if you want me to revert this change.

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.

LGTM +1 for merge with or without my pytest enhancement. @rob-luke I'll let you decide whether or not to do it, depends on how much you want to dig into pytest magic :)

@rob-luke
Copy link
Member Author

rob-luke commented Mar 9, 2020

Ok don’t merge. And I will try this fancy pytest stuff.

@rob-luke
Copy link
Member Author

rob-luke commented Mar 9, 2020

Thanks @larsoner I think I have implemented your suggestion. Can you please review again.

@larsoner larsoner merged commit a2d1461 into mne-tools:master Mar 9, 2020
@larsoner
Copy link
Member

larsoner commented Mar 9, 2020

Thanks @rob-luke !

@larsoner larsoner added this to the 0.20 milestone Mar 9, 2020
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