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

[fNIRS] use .nosatflags_wlX in case they exist #7926

Closed
swy7ch opened this issue Jun 25, 2020 · 13 comments
Closed

[fNIRS] use .nosatflags_wlX in case they exist #7926

swy7ch opened this issue Jun 25, 2020 · 13 comments

Comments

@swy7ch
Copy link
Contributor

swy7ch commented Jun 25, 2020

Describe the bug

The NIRStar software sometimes puts NaN values in the .wlX files when it estimates the probes saturated. It is not clear how (or even why) it does so, but it changes the data without the approval of the researcher, which is a bad thing. The software does provide the "true" values in a separate file name .nosatflags_wlX, which is a copy of the corresponding .wlX file with the true values.

Currently, the library checks for any file in the working directory that ends with 'wl1'. When the nosatflags_wlX files exist, the glob.glob() function returns a list of two paths, one for .wlX and another for .nosatflags_wlX, and then an error is raised saying :

'Expect one wl1 file, got 2'

Steps to reproduce

  • Get a "saturated" sample of data
  • Read the data from said sample

Expected results

Warn the user that we don't use the .wlX file but the corresponding .nosatflags_wlX one instead, and use it.

Actual results

An error is raised saying there is too much .wlX files.

Additional information

Platform: Windows-10-10.0.18362-SP0
Python: 3.8.3 (default, May 19 2020, 06:50:17) [MSC v.1916 64 bit (AMD64)]
Executable: C:\Users\juliend\Anaconda3\envs\mne_dev\python.exe
CPU: Intel64 Family 6 Model 58 Stepping 9, GenuineIntel: 8 cores
Memory: 15.6 GB

mne: 0.21.dev0
numpy: 1.18.1 {blas=mkl_rt, lapack=mkl_rt}
scipy: 1.4.1
matplotlib: 3.1.3 {backend=Qt5Agg}

sklearn: 0.22.1
numba: 0.49.1
nibabel: 3.1.0
cupy: Not found
pandas: 1.0.3
dipy: 1.1.1
mayavi: 4.7.2.dev0
pyvista: 0.25.3
vtk: 9.0.0
PyQt5: 5.9.2

--

I have a patch that warns the user of the switch in used files, and also use the right one, that you can find here (I will PR it, just wanted to open an issue before as I didn't know the right worflow)

@larsoner
Copy link
Member

Thoughts @rob-luke ?

@rob-luke
Copy link
Member

Thanks again for another very clear issue @swy7ch. The computer system info is handy. Can you also add info about your NIRX (device name) and NIRStar version along with future issues (I am sure you mentioned the details in previous issue, but its slipped my mind)?

I've got many hundreds of files from three labs with NIRScout machines and never seen this nosatflags_wlX file. Could it be specific to your hardware? Are you using a NIRSport device? Or it may be that you are operating your machine differently to us, and that's what causes these files. Or it could be that you have different photodetectors, we have APD, what are you using? Or it could be a setting in NIRStar.

If as I suspect this is a NIRSport specific issue, then I think we have to consider if we keep plugging these issues one by one, or create a proper test set and update the nirx reader. My vote (for efficiency reasons) is to create a single PR which adds support for NIRSport1 or 2 (they appear to have quite different file formats).

@rob-luke
Copy link
Member

rob-luke commented Jun 26, 2020

Secondly we need to decide what is the better option to read and provide to users. The unsaturated data, with NaNs. Or data that is known to have saturated values in it, which we know aren't valid but don't know which values are bad. I'm not sure there is a correct answer here, it probably depends on the particular study. So perhaps it should be a flag that's passed in to the read function. I think providing a warning as you have done is a good idea.

I think when reading data I would prefer to have the data with NaNs in it. This may break downstream processing (filtering?), but I would like to handle the saturation myself. Whereas if I read the data with saturated values I would never know which values were untrustworthy. For example I usually cut my data in to epochs, then I could just throw away epochs that had Nans in them because I know the machine saturated then. Or I could crop segments out of the original raw file that are +- a few seconds of saturated segments.

@cbrnr
Copy link
Contributor

cbrnr commented Jun 26, 2020

👍 for tackling NIRSport (1 and 2) in a separate PR; I've already created #7922 to track progress.

However, the issue of saturated values could also apply to NIRScout devices (I assume it's a setting in the recording software), so this would then be a separate PR. In any case, 👍 for adding a flag to handle saturated values instead of automatically going for one specific option.

@rob-luke
Copy link
Member

I assume it's a setting in the recording software

Ok interesting. Lets confirm this first. Once someone has identified the setting I can easily test on my machine.

@swy7ch
Copy link
Contributor Author

swy7ch commented Jun 26, 2020

Are you using a NIRSport device?

Yup. I guess the NIRSport has probe limitations due to its size and because it's meant to be moved - so it must be lighter/more resistant, I guess ?

Anyway the data was probed using a NIRSport 1 (8x8 configuration) through NIRStar 15.2

I think when reading data I would prefer to have the data with NaNs in it

This is an interesting debate. I'm fairly new to the research world but I do believe one needs total control over their data. My main problem here is that I can't find the condition for the probes to saturate : the software is basically hiding information to us (not actually hiding it since there are the nosatflags files, but you get the point).

Now there's another problem because my colleagues already processed the data with the Homer2/3 libraries for MATLAB, and they can't use files with NaN because some function legitimately return NaN as soon as they see one (for calculing an average, for example). I don't know how Python handles NaN but this could simply nullify a whole experiment because of one single NaN value.

Lastly, I think that using the true values instead of NaN is OK because those NaN either appeared during non-relevant, resting periods and will be discarded, or during relevant periods and will be averaged out - in group analysis at the very least.

In any case, 👍 for adding a flag to handle saturated values instead of automatically going for one specific option.

I will amend the commit to add a flag (saturated? went with nosatflags boolean) then. In the read_raw_nirx() and the RAWNirx __init__ methods I believe ? I'll open the PR after updating.

@cbrnr Any idead of what needs to be done to "add another reader" ? I'd be happy to help :)

@cbrnr
Copy link
Contributor

cbrnr commented Jun 26, 2020

So this is indeed specific to NIRSport. To "add another reader", we'd basically have to figure out the unique set of files that each device produces. Currently, this is only the case for NIRScout. I do think that these devices are still similar enough to implement everything in one reader - and yes, this means in RawNIRX.__init__.

I've linked example files for both NIRSport 1 and 2 in #7905 - we can also include them in our testing data repo. Basically, an initial version of an extended NIRX reader should be able to at least read these files correctly (and I'm sure you also have plenty of NIRSport data sets to do your own testing with).

@rob-luke
Copy link
Member

rob-luke commented Jun 26, 2020

So this is indeed specific to NIRSport.

Ok thanks for confirming this so quickly.

I suggest we add NIRSport support in a single PR rather than multiple issues. Can you take a few small test recordings and document them as in mne-tools/mne-testing-data#51

It’s handy to know all the settings that are used to create the test data so we can retrospectively check if certain behaviour (such as in this issue) are caused by device settings, and if we need more test coverage.

Test recordings with and without saturation occurring would be handy. Also recordings with whatever nirstar versions you need to support.

@swy7ch
Copy link
Contributor Author

swy7ch commented Jun 26, 2020

I can take a few recordings yes, shall I just follow the steps you took in your PR ? I'm just wondering which montage you made to have usable data. @cbrnr data doesn' seem to contain triggers

I'll create the PR for NIRSport v1 support, as suggested above by @rob-luke. NIRSport v2 support should be handled in another PR, since it doesn't provide the same data structure (at all).

@rob-luke
Copy link
Member

I'm just wondering which montage you made to have usable data.

From memory I was just making random patterns for the montage in the existing tests. Use whatever montage you prefer. Just be sure to take a screenshot of it, so that once we plot it in 3D with MNE we can verify it matches what you expect. Thanks

@swy7ch
Copy link
Contributor Author

swy7ch commented Jun 26, 2020

No problem ! I assume I'd have to set the montage on my head too ? Sorry for the newbie questions haha

@rob-luke
Copy link
Member

Probably don’t need it on your head.

@rob-luke
Copy link
Member

Closed in #9348

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

No branches or pull requests

4 participants