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 test that NIRS naming matches stored frequency #7499

Merged
merged 1 commit into from
Mar 24, 2020

Conversation

rob-luke
Copy link
Member

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

Reference issue

General fNIRS tweaks #7057.

What does this implement/fix?

In #7064 the NIRS light frequency was moved from being stored in the channel name to being stored in info['chs'][ii]['loc'][9]. However, the frequency is still stored in the channel name for ease of reading, plotting, and because MNE throws an error if two channels have the same name.

This PR adds a check that the light frequency stored in the ch_name and in loc match. And adds tests.

Additional information

People shouldn't mess with the channel names and this is specified in the docs already. https://github.com/mne-tools/mne-python/blob/master/tutorials/io/plot_30_reading_fnirs_data.py#L26

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.

Looks like reasonable sanity checking, +1 for merge if you're done @rob-luke

@rob-luke
Copy link
Member Author

Just fixed a typo. But I am done now.

@rob-luke
Copy link
Member Author

@larsoner I always prefer more checks, but I guess these things slow the code down. Whats the general preference around here?

@larsoner
Copy link
Member

I guess these things slow the code down. Whats the general preference around here?

In general more checks are better. Typically we have slowdowns in the hundreds of ms to seconds range (even plotting is here), so something like this that adds what I assume is < 1 ms is fine.

@larsoner
Copy link
Member

I should say typically we have these slowdowns when we want to do something interesting, not in checks like this...

@rob-luke
Copy link
Member Author

In general more checks are better.

Thanks.

circle is failing, but looks unrelated

@larsoner
Copy link
Member

The 3.7 pip build on Travis is probably also going to fail, both hopefully will be fixed by #7501

@rob-luke rob-luke changed the title WIP: Add test that NIRS naming matches stored frequency MRG: Add test that NIRS naming matches stored frequency Mar 24, 2020
@larsoner larsoner added this to the 0.20 milestone Mar 24, 2020
@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Merging #7499 into master will decrease coverage by 2.44%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7499      +/-   ##
==========================================
- Coverage   90.12%   87.67%   -2.45%     
==========================================
  Files         453      453              
  Lines       82661    82666       +5     
  Branches    13052    13053       +1     
==========================================
- Hits        74496    72478    -2018     
- Misses       5344     7390    +2046     
+ Partials     2821     2798      -23

@agramfort agramfort merged commit 90f1d50 into mne-tools:master Mar 24, 2020
@agramfort
Copy link
Member

thx @rob-luke

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.

None yet

3 participants