-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
WIP: Add NIRSport support #7936
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
Closed
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e7b7c37
[fNIRS]feat: add nosatflags_wlX files support
06f46c0
[fNIRS]feat: add nosatflags tests for NIRSport1
8e73ca4
[fNIRS]feat: add 'NaN' annotations
184c91e
[fNIRS]feat: add a test for data with annotated_nan
aa773c2
[fNIRS] update release number and hash following the addition of NIRS…
rderollepot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,24 @@ def test_nirx_dat_warn(tmpdir): | |
| read_raw_nirx(fname, preload=True) | ||
|
|
||
|
|
||
| @requires_testing_data | ||
| def test_nirx_nosatflags_v1_warn(tmpdir): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great. Lets update with your real data once collected. |
||
| """Test reading NIRSportv1 files with saturated data.""" | ||
| shutil.copytree(fname_nirx_15_2_short, str(tmpdir) + "/data/") | ||
| shutil.copyfile(str(tmpdir) + "/data" + "/NIRS-2019-08-23_001.wl1", | ||
| str(tmpdir) + "/data" + | ||
| "/NIRS-2019-08-23_001.nosatflags_wl1") | ||
| fname = str(tmpdir) + "/data" + "/NIRS-2019-08-23_001.hdr" | ||
| with pytest.raises(RuntimeWarning, match='specified to use the standard'): | ||
| read_raw_nirx(fname, saturated='nan', preload=True) | ||
| with pytest.raises(RuntimeWarning, match='specified to annotate your'): | ||
| read_raw_nirx(fname, saturated='annotate', preload=True) | ||
| with pytest.raises(RuntimeWarning, match='You chose to ignore them'): | ||
| read_raw_nirx(fname, saturated='ignore', preload=True) | ||
| with pytest.raises(RuntimeWarning, match='Falling back to default'): | ||
| read_raw_nirx(fname, saturated='foobar', preload=True) | ||
|
|
||
|
|
||
| @requires_testing_data | ||
| def test_nirx_15_2_short(): | ||
| """Test reading NIRX files.""" | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| # Author: David Julien <david.julien@ifsttar.fr> | ||
| # | ||
| # License: BSD (3-clause) | ||
|
|
||
| import numpy as np | ||
|
|
||
| import warnings | ||
|
|
||
| from ..annotations import Annotations | ||
| from ..utils import _mask_to_onsets_offsets | ||
|
|
||
|
|
||
| def annotate_nan(raw): | ||
| """Detect segments with NaN and return a new Annotations instance. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| raw : instance of Raw | ||
| Data to find segments with NaN values. | ||
|
|
||
| Returns | ||
| ------- | ||
| annot : instance of Annotations | ||
| Updated annotations for raw data. | ||
| """ | ||
| annot = raw.annotations.copy() | ||
| data, times = raw.get_data(return_times=True) | ||
| sampling_duration = 1 / raw.info['sfreq'] | ||
|
|
||
| nans = np.any(np.isnan(data), axis=0) | ||
| starts, stops = _mask_to_onsets_offsets(nans) | ||
|
|
||
| if len(starts) > 0: | ||
| starts, stops = np.array(starts), np.array(stops) | ||
| onsets = (starts + raw.first_samp) * sampling_duration | ||
| durations = (stops - starts) * sampling_duration | ||
| else: | ||
| warnings.warn("The dataset you provided does not contain 'NaN' " | ||
| "values. No annotation were made.") | ||
| return | ||
|
|
||
| if annot is None: | ||
| annot = Annotations(onsets, durations, 'bad_NAN') | ||
| else: | ||
| annot.append(onsets, durations, 'bad_NAN') | ||
|
|
||
| return annot | ||
swy7ch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't annotate data yet (since there is not), so I'm wondering when we should do it. Or leave the feature for later ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the data actually stored on disk as
NaN, or are they just pegged to the A/D max or min value? If they're actually stored on disk asNaN, for the first version of this reader I would just read these directly asnp.nan, and in a separate PR we can add a separate functionannotate_nanor add akwargto this reader (not sure which, let's decide later)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the data is actually stored as
NaN, and is read this way. The problems rise when one tries to plot such data.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case we shouldn't even have a
saturatedargument. The reader should just set these toNaN, and then people should make use ofmne.preprocessing.annotate_nanor something else to fix themUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I'll remove the flags in
read_raw_nirx()then. Shall I keep the warning concerningNaN, though ?I also realized the data files that are read by default are the *.nosatflags_wlX (without
NaN), not the *.wlX. Following your answer, I guess we shall let the user deal with these files and provide only one (be it *.wlX or *.nosatflags_wlX, it's up to them) ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 is already implemented (actually, the default behaviour, see this comment
2 is also implemented with the 1st commit of this PR, but leads to problems with plotting (as
matplotlibcannot plotnp.nan)If you want to implement 1 and 2 first, only the first 2 commits are relevant. I'd just need to provide some data if it is really needed.
--
3 is dealt with in this PR's 3rd commit, so I'll finish the work (in another PR if needed, no problem !)
Regarding
annotate_nan(), I'd say it's a convenient way to deal withNaNat any step of the process. We can force the annotation in the reader (even though the process would be quite the same, I think) since people will need to do that anyway if they want to plot somethingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference would be that the actual data values in the annotated ranges would be different between solution (3) and (2)-followed-by-annotate_nan. In the former they wouldn't be
nan, and in the latter they would be. Not sure if this would be helpful or meaningful for people. But yes feel free to add it if you want.It does lead to a nice way of testing things, namely that you should be able to ensure that the
raw.get_data()from (1) matches that of (3); that the annotations of (2)-plus-annotate_nanmatch that the annotations of (3); and that thenanlocations of (2) match the annotations in (2) (and implicitly (3) by the previous check).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely :D
We could wait for @rob-luke 's opinion on the subject, but I'm in to work on it : are you OK with 1, 2 and 3 as stated above by @larsoner ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@swy7ch I would say go forward with the plan. If we can get it done in the next couple of weeks it'll make it into the upcoming 0.21 release
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've come with something usable. I did test it on several "official" datasets from one of our studies, and everything seemed OK. Unfortunately I can't provide sample data, since my tutor is not here today and I don't know how to use NIRSport.
Still need to provide tests forJust realized we need sample data to perform tests, woops !preprocessing.annotate_nantoo, I'll work on it today.I'll try to get some sample data before my intership ends on Friday, and update the PR if needed. My tutor knows about the PR though, so he may be able to help you out if we're short !