MRG: Initial support for NIRS data

[rob-luke]

PR Description

This PR adds support for fNIRS. There is a BEP in progress for NIRS support here... bids-standard/bids-specification#438

This is a work in progress of a work in progress document. It should not be relied upon for storage of data.

I am maintaining this PR to stay up to date with the BEP. The plan is to use this PR to test what works and doesn't in the document, and iterate accordingly.

Merge checklist

Maintainer, please confirm the following before merging:

• All comments resolved
• This is not your own PR
• All CIs are happy
• PR title starts with [MRG]
• whats_new.rst is updated
• PR description includes phrase "closes <#issue-number>"
• Commit history does not contain any merge commits

[agramfort]

@rob-luke is the bids validator working already with fNIRS? if not I would start there

[rob-luke]

No it's not. Im working on that now too. Will focus my efforts there first then.

[sappelhoff]

cross link: https://github.com/bids-standard/bids-validator/pull/952

👍 good idea to open PRs for this early @rob-luke

[jasmainak]

You will need the validator for testing in mne-bids

[sappelhoff]

You will need the validator for testing in mne-bids

but you can use your own bids-validator fork for testing in this mne-bids PR ... that way you get your own experimental bids-validator nirs support for the changes you want to introduce here.

You'd have to change this line:

mne-bids/.travis.yml

Lines 38 to 39 in 1ce1717

	# can be "stable", or anything that can be used with git checkout
	- VALIDATOR_VERSION="8887074e5f5f48a768842ffcf96fb2952dbe0e38"

and this one, to point to your fork:

mne-bids/.travis.yml

Line 69 in 1ce1717

git clone https://github.com/bids-standard/bids-validator

and similarly in Appveyor.

PS: But of course we can only merge here, once your validator PR is merged. (So this PR will probably also have to be rebased regularly)

[rob-luke]

But of course we can only merge here, once your validator PR is merged. (So this PR will probably also have to be rebased regularly)

Absolutely. Thanks

you can use your own bids-validator fork for testing in this mne-bids PR

This is some dark git magic. Love it, thanks for the pointer.

[rob-luke]

Ok I will get back to this soon, and looks like I will need to rebase. I think most of the behind the scenes work for snirf is now completed at mne-tools/mne-python#8079 and mne-tools/mne-nirs#151 and mne-tools/mne-python#7972

[jasmainak]

sounds good!

[codecov-io]

Codecov Report

Merging #406 (a6bf4c7) into master (2cceb73) will decrease coverage by 1.70%.
The diff coverage is 26.38%.

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
- Coverage   93.98%   92.27%   -1.71%     
==========================================
  Files          23       23              
  Lines        2791     2860      +69     
==========================================
+ Hits         2623     2639      +16     
- Misses        168      221      +53     

Impacted Files	Coverage Δ
mne_bids/dig.py	76.95% <10.25%> (-13.62%)	⬇️
mne_bids/write.py	94.45% <34.61%> (-2.66%)	⬇️
mne_bids/utils.py	95.40% <50.00%> (-0.53%)	⬇️
mne_bids/config.py	97.50% <100.00%> (+0.09%)	⬆️
mne_bids/read.py	97.87% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2950db...a6bf4c7. Read the comment docs.

[codecov-commenter]

Codecov Report

Merging #406 (387c606) into main (b26f062) will increase coverage by 0.10%.
The diff coverage is 98.73%.

@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
+ Coverage   94.63%   94.74%   +0.10%     
==========================================
  Files          25       25              
  Lines        3600     3672      +72     
==========================================
+ Hits         3407     3479      +72     
  Misses        193      193              

Impacted Files	Coverage Δ
mne_bids/path.py	97.67% <ø> (ø)
mne_bids/utils.py	96.11% <83.33%> (-0.42%)	⬇️
mne_bids/config.py	97.56% <100.00%> (+0.09%)	⬆️
mne_bids/dig.py	92.79% <100.00%> (+1.42%)	⬆️
mne_bids/read.py	96.16% <100.00%> (ø)
mne_bids/write.py	96.81% <100.00%> (+0.19%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66a568f...387c606. Read the comment docs.

[larsoner]

@rob-luke can we move forward with this somehow? One option is that when reading fNIRS files we emit a warning that it's experimental. I think it "just" needs a rebase or -- maybe easier -- merge with main.

[rob-luke]

Getting this merged will save me a headache and simplify the tutorials in MNE-NIRS too!! That would be fantastic if we can merge this before the spec is accepted (from early comments above I got the impression this was not acceptable). I will rebase then you can add whatever warnings you'd expect.

[larsoner]

That would be fantastic if we can merge this before the spec is accepted (from early comments above I got the impression this was not acceptable)

It looks like this PR has been open for a year and a half, any additional arguments you can make in its favor based on that to persuade folks? Like it's needed to read data from some studies with public data in the wild, has been pretty stable, etc.? At some point it seems like merging something experimental might be worth it if the spec expansion is moving very slowly, but not expected to deviate much from what this PR does.

But I am far from a BIDS expert, so I might be way off on this...

[rob-luke]

I think we want to get this merged because the spec is stable (we are just writing the manuscript now), has been implemented as is in the other toolboxes (Homer), is used for several publicly available studies.

But I am having a hard time rebasing. I might just make a new branch from main and copy the changes. Its a mess for such old PRs :( Unless you have another tip?

[larsoner]

Rebasing 43 commits is a nightmare. I would git rebase -i HEAD~43, squash all but one commit, then git fetch upstream && rebase upstream/main. Then you only have to fight with one set of file conflicts, not one set of file conflicts per (worst case) each of the 43 commits.

[larsoner]

... but sometimes copy-paste is easy enough. Since this is just -16, it seems like that would hopefully be pretty easy as you're not modifying much existing code.

Either way, though, you could force-push here rather than opening a separate PR. For example:

git checkout nirs
git reset --hard origin/nirs
git push origin nirs:nirs-bak
git reset --hard upstream/main
... # do your copy-paste-commit
git push origin --force nirs

Then this PR just sees your one new commit, but preserves the conversation.

[rob-luke]

I started adding some tests, but we will need to wait for mne-tools/mne-testing-data#91 to be merged first.

[rob-luke]

Great! The first test is passing on mne-main, validator-main (my branch) with the new testing data.
I will add a few more tests and look through the code myself (2020 was a long time ago 😱 ). Then I will ping when ready for more eyes

[rob-luke]

I have a minimal example running. But I did not have time to write the associated text yet, I will do that in the coming days. In the meantime, I had one question about how to download the data correctly.

[hoechenberger]

@rob-luke Could you please make it such that the originally downloaded and the created BIDS files end up in ~/mne_data somewhere (I think that's what we do in the other examples) so it'll be easier for users to actually find and browse those files?

[rob-luke]

Thanks for the feedback, you can see the latest render of the example at https://6055-89170358-gh.circle-artifacts.com/0/dev/auto_examples/convert_nirs_to_bids.html

[hoechenberger]

LGTM!

[hoechenberger]

Thanks @rob-luke!

[rob-luke]

Thank you @hoechenberger!